-
Notifications
You must be signed in to change notification settings - Fork 45
Conversation
# and also avoids problems such as 3.1.12 not being available on arm | ||
def supported_etcd_arch_and_version(): | ||
return [ | ||
[ "amd64", "3.1.12" ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add ("amd64", "2.2.1") ... I know we don't want to support 2.2.1 for much longer (if at all) but it would make this PR strictly additive.
Of course I may be forgetting why we removed it!
(And no need to support on arm64, there's no user base on arm64/2.2.1 that we need to cater to)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to do this, but will not be able to test.
I removed it purely because of build issues. v2.2.1 is built from source and and my Bazel version doesn't seem to like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok to skip 2.2.1 here as we build the layer differently. The latest commit should fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - makes sense, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
Looks good in general. If we can continue to support 2.2.1 then this can be additive. I'll look into the manifest tool options; I believe our fallback position is to dual-tag the amd64 image as etcd-manager and etcd-manager-amd64. |
Thanks for checking this @justinsb. Will update this today, but will need to sync on the image/repo names to make multiarch manifest easier in DockerHub. |
9d5c51e
to
98e5b6f
Compare
386fa33
to
552736b
Compare
Just merged #345 - I think this needs a rebase, but hopefully we're now unblocked on this PR? |
@justinsb Actually it was unblocked until #345 was merged :). |
layers = select({ | ||
"@io_bazel_rules_go//go/platform:amd64": [ | ||
"etcd-2.2.1-layer-amd64", | ||
"etcd-3.1.12-layer-amd64", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may be able to generate (most) of this from supported_etcd_arch_and_version ... an idea for the future :-)
Looks good - let's get this merged :-) Thanks @hakman /approve |
Thanks @justinsb :) |
Taking #315 a bit further.
The manifest part can be done in 2 ways separately:
docker manifest ...
:$ export DOCKER_CLI_EXPERIMENTAL=enabled $ docker manifest create \ kopeio/etcd-manager:docker \ kopeio/etcd-manager-amd64:latest kopeio/etcd-manager-arm64:latest $ docker manifest push --purge kopeio/etcd-manager:docker
manifest-tool ...
: