Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Enable multi-arch build for cilium-cli image #2782
base: main
Are you sure you want to change the base?
Enable multi-arch build for cilium-cli image #2782
Changes from all commits
aed83dc
f996cce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
let's run step only on tag push, not branch push. https://quay.io/repository/cilium/cilium-cli?tab=tags should only contain released images 🙏
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.
@michi-covalent maybe
github.ref_type == 'tag' && github.event_name == 'tag'
?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.
This was never changed. Happy to change it, but didn't want to modify the current pipeline behavior.
Let me know and I can perform the update.
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'm not even sure if you want to change it, because this would allow the release of the image on merges to main using the commit hash.
See https://github.com/cilium/cilium-cli/pull/2782/files#diff-d0a3d6684c78a148cbf0725d5fe8b5aab6431da05b698a82c9e015516f3020baR48-R57
So changing this would change the existing behavior as that step output is used during the Build here https://github.com/cilium/cilium-cli/pull/2782/files#diff-d0a3d6684c78a148cbf0725d5fe8b5aab6431da05b698a82c9e015516f3020baR99
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.
the latest workflow file looks good to me. just wanted to make sure we don't push to quay.io/cilium/cilium-cli on push to main branch 🚀🙏
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.
hmm don't we still end up pushing to quay.io/cilium/cilium-cli repo on push to main?
cilium-cli/.github/workflows/images.yaml
Lines 64 to 77 in f996cce
i suggest keeping this pull request for changing Dockerfile only. let's forcus on getting Dockerfile change merged in this PR. @tklauser and i will set up a release image workflow. we need to create a separate github environment anyways.
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'll do something like this for the release image workflow once the Dockerfile change gets merged => c6cfc8b
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.
@michi-covalent the separete github env is already created based on earlier feedback.
https://github.com/cilium/cilium-cli/pull/2782/files#diff-d0a3d6684c78a148cbf0725d5fe8b5aab6431da05b698a82c9e015516f3020baR23
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 didn't mean to update the workflow file to refer to a release environment. i need to configure a github environment for this repo and make sure that it works.
let's limit the scope of this pull request to the Dockerfile change 🙏
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.
Without the workflow it won't work right. Hence #2782 (comment)
The Github environment you have to configure basically needs to match the name here https://github.com/cilium/cilium-cli/pull/2782/files#diff-d0a3d6684c78a148cbf0725d5fe8b5aab6431da05b698a82c9e015516f3020baR31
Guess the only thing you need to do is configure the secrets for publishing the image int there.
Why don't you add the environment, (give it a different name to your desires) and then we update it here in the workflow.
I'm not fully getting the problem with the workflow here. Basically all required to use a different gh-env is in place. You only need to add the secret to 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.
Is this really necessary? It shouldn't be since we cross compile cilium without tonistiigi/xx as an example.
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.
This is required within Docker, to compile with CGO_ENABLED=0. Don't recall the exact reason, I think it was related to alpine based builds (muslc, vs glibc).
I also use some buildx features that allow caching and parallel builds for all the supported platforms to have a speedy and optimized build.
I contributed similar to the Spire project couple of years ago.
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.
In essence the tool does following and is basically a small little wrapper.
https://github.com/tonistiigi/xx?tab=readme-ov-file#go--cgo
It wraps the go command by setting some variables accordingly based on the buildx args.
Saves a lot of plumbing.
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.
re-requested review from andre. let's see what he says 🙏
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 can still use the caches without using this dependency. Personally, I would prefer to not have any wrappers on official builds.
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.
this isn't related to the caching only. It is also to set the go flags accordingly for crosscompile based on the TARGETPLATFORM and TARGETARCH which is a buildx feature.
It is used here https://github.com/cilium/cilium-cli/pull/2782/files#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R19
Without that we would have to do all the plumbing for that in the RUN statement. I'm not a fan of reinventing the wheel :)
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.
Small little wrapper that sets the correct go build flags and such based on TARGETPLATFORM and TARGETARCH and such.
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 also allows verifying the binary, see here.
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.
Run as non root
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.
Similar to https://github.com/cilium/cilium/blob/78b2e1a189bccd7c863715d3f7708f8fdab1cf59/cilium-cli/Dockerfile, I don't think this will work because, for example, awscli, downloaded on line 53, is for x86_64 only.
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 can tell you it does work.
https://github.com/cilium/cilium-cli/pull/2782/files#diff-d0a3d6684c78a148cbf0725d5fe8b5aab6431da05b698a82c9e015516f3020baR33
The ci image is only build for amd64. If you want to add arm64 support for that image I'm happy to another PR for that.
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.
What I meant was that it will not work for arm64 because of the binary downloaded on line 53. The build will work but the resulting image will not work for arm64.
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.
Let me state it differently, because we are not building this image for arm64 in the workflow, it won't be a problem until we change that in the workflow.
For now this PR only focusses on building the cilium image in both architectures, the ci image will stay with amd64 support only.