ROSA-745: require full prow checks for rbac-permissions-operator#80705
Conversation
openshift#79945 only required ci/prow/images alongside Konflux, so GitHub auto-merge could squash with red lint/test/coverage/validate. Add explicit DPP prow contexts to branch-protection required_status_checks. Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@MitaliBhalla: This pull request references ROSA-745 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the initiative to target the "5.0.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
approved
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
[REHEARSALNOTIFIER] Note: If this PR includes changes to step registry files ( |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughFive additional required Prow CI status check contexts ( Changesrbac-permissions-operator Branch Protection
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| - ci/prow/coverage | ||
| - ci/prow/e2e-binary-build-success | ||
| - ci/prow/images | ||
| - ci/prow/lint | ||
| - ci/prow/test | ||
| - ci/prow/validate |
There was a problem hiding this comment.
These jobs are not always run. always_run: false.
If we are mandating these tests to pass, for those PRs which skip these tests, they won't be able to merge as those tests won't run. (that's my understanding, please correct me).
I understand the intention is to let the auto merge works, while preventing it from merging the failed test PRs.
If we need to urgent fix this, possible ideas:
(1) We can set those jobs to always_run: true. It could be a little bit waste of compute resources but it doesn't hurt to test all PRs.
(2) Let the auto merge to add the lgtm/approve label, so it can follow the tide process which needs all required tests to pass before it merges.
There was a problem hiding this comment.
Thanks — you're right that lint, test, coverage, validate, and e2e-binary-build-success are always_run: false in ci-operator today.
The motivation is MintMaker #367: it merged via GitHub auto-merge while those prow jobs were red. GitHub only enforces checks listed in required_status_checks.contexts; Tide required-if-present-contexts does not block platform auto-merge.
#79945 only required Konflux + ci/prow/images, so this follow-up adds the full DPP prow set explicitly so a failed validate/lint/etc. blocks auto-merge when those jobs run.
On the skip edge case: if a PR never triggers a conditional mandatory job, GitHub may show the context as pending/expected. Mitigations if that becomes painful:
- Flip the DPP prow presubmits to
always_run: trueforrbac-permissions-operatorin a follow-up release PR (more compute, unambiguous gating). - Keep Tide +
lgtm/approvedas the merge path for PRs where auto-merge is not appropriate.
Happy to do (1) in this PR or a fast follow-up if you prefer that over the current explicit-context approach.
There was a problem hiding this comment.
@MitaliBhalla Thanks for checking!
Let's merge this one first, so you can test and validate. Could you create a follow-up PR or card to flip always_run? I want to keep that on track.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feichashao, MitaliBhalla The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@MitaliBhalla: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
@MitaliBhalla: Updated the following 2 configmaps:
DetailsIn 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-sigs/prow repository. |
Prowgen ignores always_run: true while skip_if_only_changed or run_if_changed is set. Drop those filters so presubmits match generated config and jobs run on every PR (feichashao openshift#80705 follow-up). Co-authored-by: Cursor <cursoragent@cursor.com>
…nshift#80705) openshift#79945 only required ci/prow/images alongside Konflux, so GitHub auto-merge could squash with red lint/test/coverage/validate. Add explicit DPP prow contexts to branch-protection required_status_checks. Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Summary
Follow-up to #79945: require all DPP prow presubmits on
rbac-permissions-operatormaster, not onlyci/prow/images.MintMaker PR #367 merged via GitHub auto-merge while
ci/prow/lint,ci/prow/test,ci/prow/coverage, andci/prow/validatewere red — GitHub only enforces checks listed inrequired_status_checks.contexts, not tiderequired-if-present-contexts.Required contexts (7)
Konflux kflux-prd-rh03 / rbac-permissions-operator-on-pull-requestci/prow/coverage,ci/prow/e2e-binary-build-success,ci/prow/images,ci/prow/lint,ci/prow/test,ci/prow/validateTest plan
ci/prow/validateblocks GitHub auto-mergelgtm+approvedwhen all checks greenMade with Cursor
Summary by CodeRabbit
This PR expands the branch protection requirements for the
rbac-permissions-operatorrepository's master branch by adding comprehensive prow check enforcement to the Prow configuration.What changed: The branch protection rules for
rbac-permissions-operatornow require six additional prow status checks alongside the existing Konflux check:ci/prow/coverageci/prow/e2e-binary-build-successci/prow/lintci/prow/testci/prow/validatePreviously, only
ci/prow/imageswas required. This gap allowed a PR to be auto-merged via GitHub despite failing lint, test, coverage, and validation checks.Why it matters: GitHub's branch protection enforcement only respects checks listed in
required_status_checks.contexts. By expanding this list, the configuration ensures that PRs cannot merge unless all required prow checks pass, closing a loophole that allowed previously passing PRs with failing checks to be merged automatically.Configuration affected:
core-services/prow/02_config/openshift/rbac-permissions-operator/_prowconfig.yaml(+5 lines)