-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Specify the arch passed as build arg in the distroless image (#848) #882
Specify the arch passed as build arg in the distroless image (#848) #882
Conversation
Welcome @Serializator! |
Hi @Serializator. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
I have verified locally that this fix works, great job! As for testing we should be able to confirm by checking image pushed after merge gcr.io/k8s-staging-metrics-server/metrics-server:master |
fef628b
to
c202e49
Compare
Thank you! I made the necessary changes according to your feedback. |
/ok-to-test |
Ups, looks like you need to specify default for Line 13 in 85a18b5
|
c202e49
to
7b9346f
Compare
I added "amd64" as the default 👍🏼 |
/retest |
@serathius What about adding it as a structure test in the Skaffold configuration? I looked through the kind of structure tests there are but none of them seem related to the metadata of the image, most of them seem to be ran from within the container. |
Sure, maybe instead of adding it to Skaffold tests that run k8s cluster, we might want to expand cli tests https://github.com/kubernetes-sigs/metrics-server/blob/master/Makefile#L128-L133 as they just use docker to check image. |
This should be good to merge. We can add tests as separate PR. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: serathius, Serializator 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 |
Confirmed that problem was fixed
|
What this PR does / why we need it:
I moved
ARG ARCH
to the start so that it can be used in the last stage (gcr.io/distroless/static:latest-$ARCH
). It is declared again belowFROM golang:1.17.1 as build
so that it is available whenmake metrics-server
is run.I tested with the architectures defined in the
Makefile
, each of which worked as expected.https://github.com/kubernetes-sigs/metrics-server/blob/master/Makefile#L14
"foo" and "bar" came back in the logs for
make metrics-server
as well, indicating that these are available as environment variables. I wanted to test this as well because I only noticed later thatARCH
was not available anymore when I movedARG ARCH
to the start.I don't know exactly where these images are built but it would be better to compare the architecture of the image that was built with the expected architecture, to prevent a regression in the future.
Which issue(s) this PR fixes:
Fixes #848