-
Notifications
You must be signed in to change notification settings - Fork 137
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
Adding breaking change label to container #1809
base: main
Are you sure you want to change the base?
Adding breaking change label to container #1809
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1809 +/- ##
==========================================
- Coverage 57.93% 50.19% -7.75%
==========================================
Files 50 70 +20
Lines 3119 4202 +1083
==========================================
+ Hits 1807 2109 +302
- Misses 1154 1858 +704
- Partials 158 235 +77 ☔ View full report in Codecov by Sentry. |
4a65cb0
to
831689d
Compare
Signed-off-by: Javan lacerda <[email protected]>
831689d
to
ddad4b2
Compare
run: | | ||
FORMATED_LABELS="--image-label commit-hash=$GITHUB_SHA" | ||
|
||
BRANCH_NUMBER=$(gh pr list --state all --search "sha:$GITHUB_SHA" --label "breaking-change" | awk '{print $1}') |
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.
Can we check if we need to create a PAT for this or if the default token is fine? We sometimes hit rate limits when querying GitHub. I’d rather not manage another token but just wanna check.
Cc @cpanato
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.
One other idea is to change this to fire on PR closure, like what’s documented in https://stackoverflow.com/questions/71523153/github-actions-on-pull-request-merged-with-specific-labels
You could then checkout from HEAD based on the git sha.
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.
Eh, that might not work - commit shas will differ after being squashed to merge to HEAD.
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 it should be fine as I tested this same action in my fork and it worked properly. https://github.com/javanlacerda/fulcio/actions/runs/10997017464/job/30531560844. Just hadn't the authorization to push the image, what is expected.
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 see this and will reply tomorrow (25/sept/2024) sorry !
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 sometimes hit rate limits when querying GitHub
gh pr list
does not use the "search" endpoint which I believe is the problematic one (since search rate limit is 30/minute instead of 5000/hour like others so temporary spikes can easily hit it) so I guess this is fine.
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.
if i run that locally it does not work, need to do a setup before
$ gh pr list --state all --search "sha:7920be28f7f58020c4da8ab9bb78554ad7cd67ea" --label "breaking-change"
X No default remote repository has been set for this directory.
please run `gh repo set-default` to select a default remote 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.
hmmm, that's weird because it works on my fork. Maybe because we do the checkout at line 37?
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.
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.
ok, so maybe is a local thing, not related to the gh actions env. thanks for the clarification
run: | | ||
FORMATED_LABELS="--image-label commit-hash=$GITHUB_SHA" | ||
|
||
BRANCH_NUMBER=$(gh pr list --state all --search "sha:$GITHUB_SHA" --label "breaking-change" | awk '{print $1}') |
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.
Thinking about how this will be used in PGI: Have you thought about how we will handle if multiple PRs are merged in a short period with only one labeled as a breaking change? Are we going to search back through all containers built after the last deployment?
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! The project that will deploy it will be responsible for detecting it.
Signed-off-by: Javan lacerda <[email protected]>
Signed-off-by: Javan lacerda <[email protected]>
Signed-off-by: Javan lacerda <[email protected]>
321e2be
to
1197ca9
Compare
Signed-off-by: Javan lacerda <[email protected]>
Do you think we can merge it? @cpanato @haydentherapper |
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.
Thanks for your work on this, this looks good! Just a couple small comments
@cpanato any remaining comments?
|
||
GITHUB_RUN_NUMBER ?= "local" | ||
|
||
FULL_TAG := "0.$(shell date +%Y%m%d).$(GITHUB_RUN_NUMBER)-ref.$(GIT_VERSION)" |
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.
Should this include git sha or git version?
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 when we release will be used, not sure if we want that format
GH_TOKEN: ${{ github.token }} | ||
run: | | ||
FORMATED_LABELS="--image-label commit-hash=$GITHUB_SHA" | ||
|
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.
Do we need commit hash as a label? Looks like we include that as a tag already.
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.
if that does not hurt, it is an extra thing :)
Summary
It checks and adds the
breaking-change
label to the container that is created by thebuild-container
action.It also adds a label with the
commit-hash
to the container.Release Note
Documentation
Example of a build with this configuration: