Skip to content
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

Add github actions to build and publish macvtap images. #125

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ashokpariya0
Copy link
Contributor

Add support for multiarch (linux/amd64,linux/arm64,linux/s390x) image build with GitHub actions and publish resulting images to quay.io registry.

Signed-off-by: Ashok Pariya [email protected]

What this PR does / why we need it:
The Macvtap CNI image, available at [quay.io/kubevirt/macvtap-cni](https://quay.io/repository/kubevirt/macvtap-cni?tab=tags&tag=latest), currently supports only the amd64 architecture.
This PR introduces the necessary changes for multi-platform container image building and pushes for the macvtap-cni project, including updates to GitHub Actions, Makefile, and Dockerfile to support building and pushing the image for multiple architectures (amd64, arm64, s390x).

Special notes for your reviewer:

Release note:

None

Add support for multiach (linux/amd64,linux/arm64,linux/s390x) image build
with GitHub actions and publish resulting images to quay.io registry.

Signed-off-by: Ashok Pariya [email protected]
@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 6, 2024
@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ashokpariya0
Once this PR has been reviewed and has the lgtm label, please assign phoracek for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 6, 2024
@kubevirt-bot
Copy link
Collaborator

Hi @ashokpariya0. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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-sigs/prow repository.

@@ -0,0 +1,55 @@
name: Push container image
Copy link
Contributor

@oshoval oshoval Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please dont add this file
we need to change this prow job instead, after the PR is merged

https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/logs/publish-macvtap-cni/1839203924632932352

a follow-up effort (or maybe pre refactor)
might be good indeed to use git actions instead, but this is up to Miguel decision
note please that if we are doing that, we will need of course to align project-infra, this repo, and CNAO
as CNAO consumes the images, this why imho it better be orthogonal follow-up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @oshoval, I see that the above Prow job uses the script publish.sh to build and push the macvtap CNI image, and it uses the make command. Are you suggesting that we can add multi-platform support(like we added for cnao nad other cnis) by make command and adding export PLATFORMS=all in publish.sh as a follow-up to the PR? This way, we would have two options: we can either continue using the Prow job here to build and publish the image, or we can switch to using GitHub Actions for the same purpose.
cc @maiqueb

Copy link
Contributor

@oshoval oshoval Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally i prefer to add the "all" and stick to prow atm,
switching to git actions require changes on more repos and personally won't have time to focus on that
it can be done as follow-up when time comes, but better not rush it please
thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sure, I will raise new PR with multi support enabled using make commands so that it can be easily work with existing prow job.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can even add the "all" in this PR imo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oshoval I created a new PR: #127 to enable multi-platform builds using the make command. This change ensures compatibility with the existing Prow jobs that handle the build and push of release images. I will add the GitHub Actions changes in a follow-up PR.

Copy link
Contributor

@oshoval oshoval Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks
Please drop the git actions, it is not good to have both atm, and not required
imo we can have just one PR which is 125 + 127 without git actions for now

later on we can consider to move to git actions, but not now please

@@ -30,7 +30,7 @@ go_sources=$(call rwildcard,cmd/,*.go) $(call rwildcard,pkg/,*.go) $(call rwildc

# Configure Go
export GOOS=linux
export GOARCH=amd64
export GOARCH=$(shell uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

due to https://github.com/kubevirt/macvtap-cni/pull/125/files#r1874637326
we might need to support all flag as done with other PRs please ?

@oshoval
Copy link
Contributor

oshoval commented Dec 8, 2024

Thanks

@kubevirt-bot
Copy link
Collaborator

PR needs rebase.

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-sigs/prow repository.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants