-
Notifications
You must be signed in to change notification settings - Fork 14
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
Docker image with git sha only for pushes on main branch #155
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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 understand Build images for main branch will trigger a build when commits are pushed to
main
. By not specifyingoperatorTag
, these builds will be taggedlatest
(default) and<commit-sha>
(added in this line).Build images for dev branches will NOT trigger a build for
main
due to theif
condition added (to make sure only pushes to feature branches will do it). However, Schedule build with latest image SHA versions will trigger another build, and by specifyingoperatorTag = <commit-sha>
, these images will have the<commit-sha>
tag only (redundantly added here) and will separate fromlatest
(previously built, on a different manifest).Q: Do we need to keep Schedule build with latest image SHA versions? Is it because it specifies
limitadorVersion
? If we are to keep it, can we please remove theoperatorTag
param there (so it assumeslatest
by default)? In this case, would we still need Build images for 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.
IMO, we probably want to keep Build images for main branch and get rid of Schedule build with latest image SHA versions. Maybe add
limitadorVersion
to the former?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 was about to delete the scheduled build with the latest image with the git sha tag. But then I realized that this build is actually the correct one. The build triggered from push to main is not correct.
We need to fix this in a following up PR
That's good enough, @guicassolato ?
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.
IDK. I don't really like that main is built twice today. I would get rid of the scheduled one right away and add
limitadorVersion: ${{ vars.LIMITADOR_SHA }}
to the build main one.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.
Oh! Too soon. I see the problem with that now... When the operand changes, nothing will rebuild the operator with the updated SHA, right? Damn!
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.
So add
limitadorVersion: ${{ vars.LIMITADOR_SHA }}
to build main and remove this line but keep both workflows?main
will build twice but at least both tags will be added and linked every time. Right?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.
Right.
The other option is to trigger build on the operator project when the operand gets a new build. But I rather prefer the nightly build approach instead. To avoid too many operator builds in the same day when the operand changes many times in the same day.
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 would like to fix that in another PR