-
Notifications
You must be signed in to change notification settings - Fork 29
ROX-31146: Reduce spam of Konflux PRs, releasers can approve #2693
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
base: master
Are you sure you want to change the base?
Conversation
`pull_request` is less insecure and should be sufficient.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2693 +/- ##
==========================================
- Coverage 27.60% 27.38% -0.23%
==========================================
Files 95 95
Lines 5422 5427 +5
Branches 2523 2548 +25
==========================================
- Hits 1497 1486 -11
- Misses 3213 3214 +1
- Partials 712 727 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Molter73
left a comment
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! Just a few small comments but nothing that would prevent merging.
| github.event.pull_request.user.login != 'red-hat-konflux[bot]' | ||
| env: | ||
| GH_TOKEN: ${{ secrets.RHACS_BOT_GITHUB_TOKEN }} | ||
| runs-on: ubuntu-latest |
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 know this is probably fine, but do we want to pin this to 24.04?
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.
Inept joke: it depends if you mean "my royal we" or by "we" you mean the Collector team.
I see that the runner is indeed pinned to ubuntu-24.04 in all jobs but these:
- https://github.com/stackrox/collector/blob/master/.github/workflows/add-new-pr-to-oss-triaging.yml#L12
- https://github.com/stackrox/collector/blob/master/.github/workflows/auto-approve.yml#L11
24.04 is fine for another three years (2029-04-25) when its Maintenance & Security Support cycle ends. Not sure what happens after.
In case of this job, there's nothing specific used from the OS so it could practically be any ubuntu flavor that GitHub provides. Therefore, the question is to you as the Collector team whether you'd prefer me to make the runner consistent here and whether you'd like me to also address that in add-new-pr-to-oss-triaging.yml and auto-approve.yml.
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.
Well, some time ago we run into a problem when the ubuntu-latest runner got updated from underneath us from 22.04 to 24.04, so suddenly had failing jobs and having to debug this is not really fun. So I am more concerned about a silent upgrade to 26.04 from breaking the workflow than I am from 24.04 going out of support or getting removed as a runner altogether.
That said, given the workflow just runs gh, latest is probably fine as I said and you can leave it as is if you want.
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 am more concerned about a silent upgrade to 26.04 from breaking the workflow than I am from 24.04 going out of support or getting removed as a runner altogether.
Understood. Pinned versions in all three workflows.
.github/CODEOWNERS
Outdated
| .github/renovate.json5 @stackrox/rhtap-maintainers | ||
| # The Konflux maintainers for ACS review all changes related to the Konflux pipelines, Dockerfiles, etc. | ||
| # Release engineers need to merge MintMaker PRs at the time of release. | ||
| # rhacs-bot needs ability to auto-approve MintMaker PRs for automated task and security updates. |
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.
| # rhacs-bot needs ability to auto-approve MintMaker PRs for automated task and security updates. | |
| # rhacs-bot needs the ability to auto-approve MintMaker PRs for automated task and security updates. |
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.
Updated the comment.
|
I have a parallel review for the same change in the StackRox repo - stackrox/stackrox#17968 |
Description
This is analogous to stackrox/scanner#2438 but for the Collector repo.
The main ideas are to make MintMaker PRs send no emails and to allow release engineers approve and merge MintMaker PRs independently.
Excessive context - in this thread https://redhat-internal.slack.com/archives/C05TS9N0S7L/p1756996368507969
Checklist
[ ] Updated documentation accordinglyAutomated testing
No change.
Testing Performed
Not really. Did testing in Scanner repo, here I just hope it works.