Mirror required status checks into classic branch protection for tide - #171
Merged
eliorerz merged 2 commits intoAug 5, 2026
Merged
Conversation
Prow's tide derives its merge-gating required contexts exclusively from
the classic branch-protection API (pkg/config/tide.go's
FromBranchProtection reads bp.RequiredStatusChecks, which maps to that
API specifically) -- it has no knowledge of repository rulesets at all.
osac-project's Prow config relies on the org-wide
tide.context_options.from-branch-protection default (no per-repo
override in openshift/release), but classic branch protection here has
never set required_status_checks -- only the ruleset did. Confirmed via
GitHub's API directly:
$ gh api repos/osac-project/osac/branches/main/protection \
--jq '.required_status_checks'
null
So tide's computed required-context set for every repo managed here has
always been empty, and it merges purely on labels
(approved+jira/valid-reference+lgtm), completely blind to e2e status.
Confirmed on a real merge: PR osac-project#85 was merged by openshift-merge-bot at
21:11:20Z while its three required e2e checks (which all eventually
passed) didn't finish until 22:03:04Z, 22:26:11Z, and 22:33:41Z --
52 to 82 minutes later.
Mirror the same contexts already enforced by the ruleset into classic
branch protection's own required_status_checks block, generated from
the same var.required_status_checks input so there's still only one
list to maintain per repo, not two independently-drifting ones.
Signed-off-by: Elior Erez <eerez@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe branch protection resource conditionally configures classic required status checks from ChangesBranch protection checks
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/common_repository/main.tf (1)
117-123: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign branch protection with the repository default branch. If the default branch is not
main,github_branch_protection.repo_protectiondoes not mirror the contexts enforced bygithub_repository_ruleset.status_checkson~DEFAULT_BRANCH. Derivepatternfrom the repository default branch or enforcemainas the default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/common_repository/main.tf` around lines 117 - 123, Update the github_branch_protection.repo_protection configuration to target the repository’s actual default branch by deriving its pattern from the repository default-branch value, or explicitly enforce main as that default. Ensure its protected contexts remain aligned with github_repository_ruleset.status_checks on ~DEFAULT_BRANCH.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/common_repository/main.tf`:
- Around line 104-116: Add the required Red Hat AI attribution trailer to the
commit message, using either Assisted-by: or Generated-by:. Do not use an AI
Co-Authored-By: trailer.
---
Nitpick comments:
In `@modules/common_repository/main.tf`:
- Around line 117-123: Update the github_branch_protection.repo_protection
configuration to target the repository’s actual default branch by deriving its
pattern from the repository default-branch value, or explicitly enforce main as
that default. Ensure its protected contexts remain aligned with
github_repository_ruleset.status_checks on ~DEFAULT_BRANCH.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 60ec4012-4a36-4b55-9938-2be49f924142
📒 Files selected for processing (1)
modules/common_repository/main.tf
Comment on lines
+104
to
+116
| # Prow's tide only derives its own merge-gating required contexts from | ||
| # this (classic) branch-protection API -- it has no knowledge of | ||
| # repository rulesets at all (confirmed against tide's actual source, | ||
| # pkg/config/tide.go: FromBranchProtection reads bp.RequiredStatusChecks, | ||
| # which maps to this exact API, not rulesets). Since every repo here | ||
| # already relies on org-wide tide.context_options.from-branch-protection | ||
| # (the global default in openshift/release, no per-repo override), tide | ||
| # was merging PRs on labels alone the moment none of these contexts were | ||
| # ever mirrored here -- confirmed on a real PR (#85) that merged with all | ||
| # 3 required e2e checks still 50+ minutes from completing. Mirror the | ||
| # same contexts already enforced by the ruleset below so tide actually | ||
| # waits, without introducing a second, independently-maintained list -- | ||
| # both come from the same var.required_status_checks. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git log -1 --format='%B'Repository: osac-project/github-config
Length of output: 1637
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- commit metadata ---'
git log -1 --format=fuller
printf '%s\n' '--- target module files ---'
git ls-files 'modules/common_repository/*'
printf '%s\n' '--- main.tf relevant section ---'
sed -n '85,145p' modules/common_repository/main.tf
printf '%s\n' '--- variables.tf relevant declarations ---'
rg -n -A18 -B5 'required_status_checks|integration_id|DEFAULT_BRANCH' modules/common_repository/variables.tf modules/common_repository/main.tf
printf '%s\n' '--- module callers and required checks ---'
rg -n -A12 -B8 'common_repository|required_status_checks|integration_id' --glob '*.tf' --glob '*.yaml' --glob '*.yml' .Repository: osac-project/github-config
Length of output: 40315
Add the required AI attribution trailer.
If an AI tool contributed to this commit, add a Red Hat Assisted-by: or Generated-by: trailer. Do not add an AI Co-Authored-By: trailer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/common_repository/main.tf` around lines 104 - 116, Add the required
Red Hat AI attribution trailer to the commit message, using either Assisted-by:
or Generated-by:. Do not use an AI Co-Authored-By: trailer.
Source: Coding guidelines
Signed-off-by: Elior Erez <eerez@redhat.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Why
Prow's
tidemerges PRs based purely on labels here, blind to e2e status entirely -- confirmed on a real merge, not theorized.osac-project/osac#85 was merged byopenshift-merge-bot[bot]at21:11:20Z, while all three of its required e2e checks (e2e-bmaas-full-install / e2e,e2e-vmaas-full-install / e2e,e2e-caas-full-install / e2e) were still running -- they didn't complete until22:03:04Z,22:26:11Z, and22:33:41Zrespectively (52-82 minutes after the merge). All three eventually passed, but only well after the fact.Root cause, confirmed at the source level, not guessed:
osac's Prow config relies on the org-widetide.context_options.from-branch-protectiondefault (no per-repo override inopenshift/release) to derive which contexts tide should wait for. Checked tide's actual implementation (kubernetes-sigs/prow,pkg/config/tide.go):bp.RequiredStatusChecksmaps directly to GitHub's classic branch-protection API -- there's no repository-rulesets-aware code path anywhere in this function. And classic branch protection onosac/mainhas never hadrequired_status_checksset -- only the newer ruleset (github_repository_ruleset.status_checks) does:So tide's computed required-context set for every repo managed by this Terraform has always been empty. It merges the instant
approved+jira/valid-reference+lgtmlabels are present, with no blocking labels -- it was never actually waiting for e2e, on any PR, ever. Most PRs happen to look compliant only because reviewers naturally wait to see CI pass before approving, as a matter of habit, not because anything enforces it.(Checked 5 other recently-merged PRs for comparison: 4 were fully compliant, e2e finishing well before merge. The one other match,
#170, turned out to be an unrelated, legitimate mechanism -- merged directly by a humanwg-infrateam member via the UI, using their own explicitly-configured ruleset bypass rights, not tide.)What
Mirror the same contexts already enforced by
github_repository_ruleset.status_checksintogithub_branch_protection.repo_protection's ownrequired_status_checksblock -- both generated from the samevar.required_status_checksinput, so there's one list to maintain per repo, not two independently-drifting ones.strict = truematches the ruleset's ownstrict_required_status_checks_policy = true.Why this over the alternative (editing openshift/release directly)
Considered setting
context-options.required-contextsexplicitly inosac's own_prowconfig.yamlinopenshift/releaseinstead. Went with this approach because:openshift/releasemerge rights to review.var.required_status_checksinrepositories.tf) instead of three (the workflow file names, the ruleset, and now a separate Prow config entry) -- exactly the kind of drift that caused the CaaS-Netris rename to silently break tide's gating in the first place, if it had needed a third place to update.required_status_checks, not justosac--osac-installerandfulfillment-servicehave the same latent gap today, just not yet observed on a real PR.Blast radius
Only affects repos where
required_status_checksis non-empty inrepositories.tf(currentlyosac,osac-installer,fulfillment-service). No change for repos without required checks configured (thedynamicblock stays empty for them, matching the existing ruleset's own conditional).Behavioral change to expect: any PR currently sitting with
approved+lgtmbut incomplete e2e will stop being tide-mergeable until those checks actually finish. This was already GitHub's own ruleset-enforced behavior for a human clicking "Merge" in the UI -- this closes the gap specifically for tide's automated merges, which is where it was actually being skipped.Test plan
tofu init -backend=false && tofu validate-- passes (same pre-existing deprecation warnings as always)gh api repos/osac-project/osac/branches/main/protection --jq '.required_status_checks'that it reflects the three e2e contexts once appliedSummary by CodeRabbit