-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Makefile: Push a single multi-arch image #1190
Conversation
Linking this issue here as well: #1089 |
Restarted the job, seems like e2e tests failed. |
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.
You are changing the make targets we use in our e2e tests, hence the failures.
These are the make targets we call in our e2e tests and need to continue to function correctly. https://github.com/kubernetes/kube-state-metrics/blob/master/tests/e2e.sh#L108-L114
Thanks for debugging. Good catch! Will look into what needs to be changed there |
Matching patterns from https://github.com/kubernetes-sigs/metrics-server/blob/master/Makefile Signed-off-by: Manuel Rüger <manuel@rueg.eu>
* DRY on image name * Enable e2e testing possibility on other architectures as well * Replace correct image name in manifest * Don't build binary anymore, it's now included in the image build process Signed-off-by: Manuel Rüger <manuel@rueg.eu>
@lilic adjusted e2e tests and extended them to support other architectures as well, also figured out that we don't need to build the binary twice in the e2e tests (one regular, one in container) anymore. |
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 to me. I tested it by running REGISTRY=quay.io/paulfantom make push
and using resulting images in multi-arch (arm, arm64, amd64) cluster in thaum-xyz/ankhmorpork#39. Everything seems to be working as expected.
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.
Thanks for your PR 🎉
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lilic, mrueg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sadly this caused the pipeline build to fail for us:
|
For push we must use the following as its how it is build in gcr:
Are you willling to make the fixes for this? @mrueg |
Sure will fix.
needs the experimental docker cli enabled. |
@mrueg @lilic @paulfantom Just looking at this in relation to #1037. Will this work? |
Sorry ignore me. Whilst the base image isn't multi-arch it doesn't seem to actually contain anything architecture specific, so this approach does work for metrics-server and should work here. Sorry for the doubt... |
Matching patterns from https://github.com/kubernetes-sigs/metrics-server/blob/master/Makefile
What this PR does / why we need it:
@serathius @lilic This adopts some targets of the Makefile of metrics-server's project to support a multi-arch image via test-infra's image build jobs.