Support github_merge_blocks_policy in per-repo supplemental prow config - #5354
Support github_merge_blocks_policy in per-repo supplemental prow config#5354kaovilai wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add support for configuring tide's github_merge_blocks_policy on a per-org/repo basis via supplemental _prowconfig.yaml files, following the same sharding pattern used for merge_method. Changes: - Add GitHubMergeBlocksPolicy field to the TideConfig struct in shardprowconfig, enabling per-repo config via _prowconfig.yaml - Add sharding logic in ShardProwConfig() to extract GitHubMergeBlocksPolicyMap entries from the global config into per-org/repo shard configs - Update vendored prow config mergeFrom() to recognize github_merge_blocks_policy as an allowed supplemental config field - Add merge logic for GitHubMergeBlocksPolicyMap in Tide.mergeFrom() - Add round-trip test cases for the new field Closes: openshift#5353 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthrough
ChangesGitHub merge-block policy sharding
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 17✅ Passed checks (17 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)
pkg/api/shardprowconfig/shardprowconfig.go (1)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
GitHubMergeBlocksPolicy.Add a Go doc comment that defines the supported scopes and its serialized policy purpose.
🤖 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 `@pkg/api/shardprowconfig/shardprowconfig.go` around lines 34 - 36, Add a Go doc comment above the GitHubMergeBlocksPolicy field to document its purpose and supported scopes. The comment should explain what the policy controls and which scopes are valid for configuring GitHub merge blocks behavior in the serialized JSON structure.Sources: Coding guidelines, Path instructions
🤖 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 `@pkg/api/shardprowconfig/shardprowconfig.go`:
- Around line 98-112: The GitHubMergeBlocksPolicyMap sharding loop must preserve
the global "*" entry in the root config. In the loop over
pc.Tide.GitHubMergeBlocksPolicyMap, skip orgOrRepo == "*" before extracting or
sharding it, matching the SlackReporterConfigs handling; retain normal sharding
and deletion for scoped entries. Add a round-trip test with a wildcard policy
and verify no shard file is emitted for that entry.
---
Nitpick comments:
In `@pkg/api/shardprowconfig/shardprowconfig.go`:
- Around line 34-36: Add a Go doc comment above the GitHubMergeBlocksPolicy
field to document its purpose and supported scopes. The comment should explain
what the policy controls and which scopes are valid for configuring GitHub merge
blocks behavior in the serialized JSON structure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| for orgOrRepo, policy := range pc.Tide.GitHubMergeBlocksPolicyMap { | ||
| org, repo, _ := prowconfigutils.ExtractOrgRepoBranch(orgOrRepo) | ||
| orgRepo := prowconfig.OrgRepo{Org: org, Repo: repo} | ||
| if configsByOrgRepo[orgRepo] == nil { | ||
| configsByOrgRepo[orgRepo] = &ProwConfigWithPointers{} | ||
| } | ||
| if configsByOrgRepo[orgRepo].Tide == nil { | ||
| configsByOrgRepo[orgRepo].Tide = &TideConfig{} | ||
| } | ||
| if configsByOrgRepo[orgRepo].Tide.GitHubMergeBlocksPolicy == nil { | ||
| configsByOrgRepo[orgRepo].Tide.GitHubMergeBlocksPolicy = map[string]prowconfig.GitHubMergeBlocksPolicy{} | ||
| } | ||
| configsByOrgRepo[orgRepo].Tide.GitHubMergeBlocksPolicy[orgOrRepo] = policy | ||
| delete(pc.Tide.GitHubMergeBlocksPolicyMap, orgOrRepo) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the shardprowconfig.go file around lines 98-112
sed -n '85,125p' pkg/api/shardprowconfig/shardprowconfig.goRepository: openshift/ci-tools
Length of output: 1648
🏁 Script executed:
#!/bin/bash
# Check the broader context of this function to understand what it does
ast-grep outline pkg/api/shardprowconfig/shardprowconfig.goRepository: openshift/ci-tools
Length of output: 751
🏁 Script executed:
#!/bin/bash
# Search for related test files to understand the expected behavior
find . -name "*shardprowconfig*test*" -o -name "*test*shardprowconfig*" | head -20Repository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Check what GitHubMergeBlocksPolicyMap is used for elsewhere
rg "GitHubMergeBlocksPolicyMap" --type go -B 2 -A 2Repository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Get the full function to see the complete logic
sed -n '39,180p' pkg/api/shardprowconfig/shardprowconfig.goRepository: openshift/ci-tools
Length of output: 5615
🏁 Script executed:
#!/bin/bash
# Search for Slack reporter logic and TideConfig usage
rg -A 5 "SlackReporterConfigs\|GitHubMergeBlocksPolicy" pkg/api/shardprowconfig/ -t goRepository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Look for wildcard constant definitions
rg "TideRepoMergeTypeWildcard\|Wildcard" pkg/ -t go | head -20Repository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Check if there are any tests in the same package
ls -la pkg/api/shardprowconfig/Repository: openshift/ci-tools
Length of output: 339
🏁 Script executed:
#!/bin/bash
# Search in test directories
find pkg/api/shardprowconfig -name "*test.go" -exec cat {} \;Repository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Check if there are any integration tests that cover this function
find . -path "./test/*" -name "*.go" | xargs grep -l "ShardProwConfig\|GitHubMergeBlocksPolicy" 2>/dev/null || trueRepository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Verify the wildcard constant and understand its semantics
rg "const.*Wildcard" pkg/ -A 2 -B 2 --type goRepository: openshift/ci-tools
Length of output: 415
🏁 Script executed:
#!/bin/bash
# Check prowconfig package to confirm GitHubMergeBlocksPolicy type
rg "type GitHubMergeBlocksPolicy" --type go -A 5Repository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Look at the SlackReporterConfigs comment and surrounding context for rationale
sed -n '145,165p' pkg/api/shardprowconfig/shardprowconfig.goRepository: openshift/ci-tools
Length of output: 764
🏁 Script executed:
#!/bin/bash
# Check what imports are used and verify Prow config types
head -30 pkg/api/shardprowconfig/shardprowconfig.goRepository: openshift/ci-tools
Length of output: 1128
🏁 Script executed:
#!/bin/bash
# Verify the exact code pattern - search for how wildcard is handled elsewhere
rg "prowconfigutils.TideRepoMergeTypeWildcard\|== \"\\*\"" pkg/api/ -B 2 -A 2 --type goRepository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Check the entire loop context around SlackReporterConfigs to understand the pattern better
sed -n '158,175p' pkg/api/shardprowconfig/shardprowconfig.goRepository: openshift/ci-tools
Length of output: 662
🏁 Script executed:
#!/bin/bash
# Check if there's any special handling of "*" in the output/serialization
rg "GitHubMergeBlocksPolicy\|MergeBlocksPolicy" . --type go | head -20Repository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Look for any Prow config validation or documentation
rg "merge.*block\|GitHubMergeBlocks" --type go -i | grep -i comment -A 2 -B 2 || trueRepository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Verify the exact deletion pattern and confirm no filtering exists for GitHubMergeBlocksPolicyMap
sed -n '98,115p' pkg/api/shardprowconfig/shardprowconfig.goRepository: openshift/ci-tools
Length of output: 893
🏁 Script executed:
#!/bin/bash
# Check if there are any tests at all that exercise ShardProwConfig with these fields
find . -name "*.go" -type f | xargs grep -l "ShardProwConfig" 2>/dev/null | head -10Repository: openshift/ci-tools
Length of output: 340
🏁 Script executed:
#!/bin/bash
# Look at the PR context - what changed in this area?
git log --oneline -20 -- pkg/api/shardprowconfig/shardprowconfig.go 2>/dev/null || echo "Git history not available"Repository: openshift/ci-tools
Length of output: 688
🏁 Script executed:
#!/bin/bash
# Look at the recent commit that added GitHubMergeBlocksPolicy support
git show ee437b5cc5 --stat | head -50Repository: openshift/ci-tools
Length of output: 1553
🏁 Script executed:
#!/bin/bash
# Check the main_test.go to see if it tests GitHubMergeBlocksPolicy
grep -A 20 -B 5 "GitHubMergeBlocksPolicy\|MergeBlocks" cmd/determinize-prow-config/main_test.go || echo "Not found in test"Repository: openshift/ci-tools
Length of output: 1624
🏁 Script executed:
#!/bin/bash
# Look at how the slackReporterConfigs pattern was implemented (from the git log commit)
git show 023620f5ca --stat | head -50Repository: openshift/ci-tools
Length of output: 990
🏁 Script executed:
#!/bin/bash
# Check if there's a test case for wildcard in the GitHubMergeBlocksPolicy tests
grep -B 5 -A 30 'name.*".*\*' cmd/determinize-prow-config/main_test.go | grep -A 30 "GitHubMergeBlocksPolicy" || echo "No wildcard test found"Repository: openshift/ci-tools
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Verify the exact test structure by viewing the full test case we saw
sed -n '1,50p' cmd/determinize-prow-config/main_test.go | head -50Repository: openshift/ci-tools
Length of output: 1363
🌐 Web query:
Prow github_merge_blocks_policy wildcard support configuration
💡 Result:
In Prow, the github_merge_blocks_policy configuration option supports the use of a wildcard () to apply a policy globally across all organizations and repositories [1][2]. This configuration controls how Tide handles GitHub's mergeStateStatus (e.g., when a pull request is blocked by branch protection rules, rulesets, or required reviews) [1][2]. You can define the policy at the organization level, the organization/repository level, or globally using the asterisk () key [2]. The available policy values are: * ignore: Tide ignores the BLOCKED status and attempts to merge the PR regardless [1][2]. * permit: Tide allows merging even if the PR is in a BLOCKED state, but it will log warnings and include information in the status (e.g., "In merge pool (despite BLOCKED)") [1][2]. This is the default policy [1][2]. * block: Tide respects the BLOCKED status and prevents the PR from being merged [1][2]. Example usage in your Prow configuration: tide: github_merge_blocks_policy: "*": block my-org/my-repo: permit For more details, you can refer to the official Prow configuration documentation within the source repository [2].
Citations:
- 1: tide: add configurable GitHub merge blocks enforcement kubernetes-sigs/prow#579
- 2: https://github.com/kubernetes-sigs/prow/blob/main/pkg/config/prow-config-documented.yaml
Keep the global "*" policy in the root config and add a test for it.
Prow supports "*" as the key for a global github_merge_blocks_policy. The loop at lines 98–112 shards all entries including "*" and deletes them from pc.Tide.GitHubMergeBlocksPolicyMap. This removes the global policy from the root config, preventing it from applying as intended.
The SlackReporterConfigs loop in the same function (line 159) demonstrates the correct pattern: it explicitly skips the "*" entry with if orgOrRepo == "*" { continue } and includes the explanation "Value of "*" is for applying global configurations, so no need to shard it."
Apply the same check to the GitHubMergeBlocksPolicyMap loop to preserve the global policy. Add a round-trip test case that uses a wildcard entry and verifies that no shard file is emitted for it.
🤖 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 `@pkg/api/shardprowconfig/shardprowconfig.go` around lines 98 - 112, The
GitHubMergeBlocksPolicyMap sharding loop must preserve the global "*" entry in
the root config. In the loop over pc.Tide.GitHubMergeBlocksPolicyMap, skip
orgOrRepo == "*" before extracting or sharding it, matching the
SlackReporterConfigs handling; retain normal sharding and deletion for scoped
entries. Add a round-trip test with a wildcard policy and verify no shard file
is emitted for that entry.
|
@kaovilai: The following test failed, say
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. |
|
vendoring Prow dep and patching it is a really bad idea |
Adds
github_merge_blocks_policysupport in per-repo_prowconfig.yamlsupplemental prow config.Fixes #5353. Alternative to openshift/release#82902.
Note
Proposed by chai-bot (redhat-chai-bot), opened as draft via Claude Code.
Adds support for
github_merge_blocks_policyin per-repository supplemental_prowconfig.yamlfiles.The
determinize-prow-configtool now shards organization- and repository-level policies into the correct Tide configuration files. It combines these policies with existing settings such asMergeType.Vendored Prow configuration merging now preserves and merges
GitHubMergeBlocksPolicy. Round-trip tests cover policy-only and combined configurations.