-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
🌱 Add github action to trigger container images build #1456
🌱 Add github action to trigger container images build #1456
Conversation
3021677
to
23bb442
Compare
/test-ubuntu-integration-main |
/lgtm |
23bb442
to
c18f646
Compare
c18f646
to
3a54ea8
Compare
/hold |
152ca40
to
562464c
Compare
job_name: "metal3_keepalived_container_image_building" | ||
job_params: | | ||
{ | ||
"BUILD_CONTAINER_IMAGE_NAME": "keepalived", |
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.
How does this job know which Dockerfile to build for keepalived?
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 data is stored in this file: https://github.com/Nordix/metal3-dev-tools/blob/main/scripts/files/container_image_names.json
And logic is handled here: https://github.com/Nordix/metal3-dev-tools/blob/main/scripts/build-container-image.sh#L105
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 that file used for anything else? It seems that we could pass that information via the env, so we wouldn't need to maintain a separate file, that is then out of sync. For example, utliity-images or sles-ipa repo etc.
Same argument as having the tag config in the workflow, we could have this config in the workflow as well?
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 think you have a good argument, but there're a couple of reasons that I still prefer a centralized database of images:
- I don't like the idea of putting repo address as parameter, which is error-prone imo. Any small typo could fail the job, while we actually still have cases when we need to run the builds manually.
- Having no centralized file means we will allow build and push of any repos, not just the ones we care about. Anyone on Nordix jenkins can trigger it, it can build anything, and all will be pushed to our repository in quay.
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.
- Typo anywhere would make it fail. You can probably get it via GH reference, same as branch anyways.
- I don't think they can run builds. You need group membership to launch them. As anon, I cannot access any build option. If it was possible, our current jobs could be equally abused as they're all parametrized.
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.
- Yes, but typo in url is generally harder to check. And I think the bigger problem than failed jobs is jobs that do the wrong things, and generally more params = more errors. That's of course not a problem with github triggered runs, but we still have manual builds. It can happen that we paste one url while were meant to use another: say we want to build bmo but mistakenly use url from capm3 --> BMO image will be pushed with capm3 content, quite difficult to check.
- I agree, I'm not sure regarding of jenkins permissions, it's likely that only us can trigger those jobs. Anyway, my points regarding the "any image" still checks.
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 won't allow pushing images from manual builds, so that is non issue.
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.
How do you check that, if I may ask? And if I recall correctly, one of the requirements was that the workflow should allow custom builds should it's needed.
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 cleared off-line.
We do not do manual or custom builds that are built to Quay. We define the tag patterns in the workflow file, and the typical list is:
- main/latest for main branch updates
- release-* tags for release branch updates
- v* tags or capm3-v* tags for releases
We do keep X number of builds that are timestamped + sha encoded for the branch builds.
We do not want to build lightweight tags like:
- apis/v*
And we definitely do not support building and pushing images based on user input that might mess up with the above mentioned "official" images.
562464c
to
aa11e05
Compare
aa11e05
to
bf49fd0
Compare
bf49fd0
to
8c99098
Compare
/unhold |
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.
/lgtm
/override test-centos-e2e-integration-main test-ubuntu-integration-main |
@tuminoid: Overrode contexts on behalf of tuminoid: test-centos-e2e-integration-main, test-ubuntu-integration-main In response to this:
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. |
/override metal3-bmo-e2e-test |
@tuminoid: Overrode contexts on behalf of tuminoid: metal3-bmo-e2e-test In response to this:
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. |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest 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 |
Disabled build triggers in Quay for BMO and keepalived. It should now all flow thru this workflow + Jenkins. |
This PR adds an action to trigger the bmo and keepalived container builds whenever a PR is merged in this repo.