OSAC-2333: Enable merge queues on core repos#134
Conversation
Adds a `merge_queue` variable to the common_repository module and enables it on fulfillment-service, osac-operator, osac-aap, osac-installer, and osac-test-infra. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The merge queue automatically tests PRs against the latest main, making strict (require branch to be up-to-date) redundant and adding unnecessary rebase friction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (4)
WalkthroughThe repository module now supports configurable strict status checks and optional GitHub merge queue rulesets. Five repository configurations enable merge queues and set strict status checks to false. ChangesRepository merge queue configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RepositoryModule
participant GitHubRepository
participant GitHubRuleset
RepositoryModule->>GitHubRepository: Create repository
RepositoryModule->>GitHubRuleset: Create active merge_queue ruleset
GitHubRuleset->>GitHubRepository: Target default branch
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 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 101-103: Update the strict status-check assignment in the branch
protection configuration to force false whenever var.merge_queue is configured,
while preserving var.strict_status_checks when no merge queue is provided. Keep
the existing required status-check contexts unchanged.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Enterprise
Run ID: 57e38b3c-fee0-491b-b311-3005496be379
📒 Files selected for processing (3)
modules/common_repository/main.tfmodules/common_repository/variables.tfrepositories.tf
| strict = var.strict_status_checks | ||
| contexts = var.required_status_checks | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Automatically disable strict status checks when a merge queue is configured.
GitHub does not allow requiring branches to be up to date before merging when a merge queue is active. To prevent API errors and reduce the configuration burden on module consumers, consider automatically enforcing strict = false when var.merge_queue is provided.
♻️ Proposed refactor
required_status_checks {
- strict = var.strict_status_checks
+ strict = var.merge_queue != null ? false : var.strict_status_checks
contexts = var.required_status_checks
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| strict = var.strict_status_checks | |
| contexts = var.required_status_checks | |
| } | |
| required_status_checks { | |
| strict = var.merge_queue != null ? false : var.strict_status_checks | |
| contexts = var.required_status_checks | |
| } |
🤖 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 101 - 103, Update the strict
status-check assignment in the branch protection configuration to force false
whenever var.merge_queue is configured, while preserving
var.strict_status_checks when no merge queue is provided. Keep the existing
required status-check contexts unchanged.
- Validate merge_method (MERGE/SQUASH/REBASE) and grouping_strategy (ALLGREEN/HEADGREEN) at plan time instead of failing at apply - Regenerate README.md with terraform-docs to include new variables and resources Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ns/wg-infra ci/prow/unit and ci/prow/temp were never removed from required_status_checks when Prow was decommissioned for these repos; they haven't posted a status on any recent PR. Left as required checks, they would make every merge-queue entry on fulfillment-service/osac-operator/osac-aap time out and get evicted, since nothing produces that check on the merge-group ref. The new merge_queue ruleset also had no bypass_actors, unlike the classic branch protection beside it (enforce_admins = false) - rulesets don't inherit that exemption, so there was no way for org-admins or wg-infra to merge around a stuck queue. Added a Team-scoped bypass for both, resolved via data "github_team" since the ruleset API needs numeric team IDs, not slugs. Also ran tofu fmt, which fixes the currently-failing pre-commit check (required_approvals alignment drifted in the osac_installer/osac_test_infra blocks after the PR's new fields were added).
| rules { | ||
| merge_queue { | ||
| merge_method = var.merge_queue.merge_method | ||
| min_entries_to_merge = var.merge_queue.min_entries_to_merge | ||
| max_entries_to_merge = var.merge_queue.max_entries_to_merge | ||
| check_response_timeout_minutes = var.merge_queue.check_response_timeout_minutes | ||
| grouping_strategy = var.merge_queue.grouping_strategy | ||
| } | ||
| } |
There was a problem hiding this comment.
I know that if var.merge_queue == null the count will be zero. But, will TF also skip trying to make these assignments?
| rules { | |
| merge_queue { | |
| merge_method = var.merge_queue.merge_method | |
| min_entries_to_merge = var.merge_queue.min_entries_to_merge | |
| max_entries_to_merge = var.merge_queue.max_entries_to_merge | |
| check_response_timeout_minutes = var.merge_queue.check_response_timeout_minutes | |
| grouping_strategy = var.merge_queue.grouping_strategy | |
| } | |
| } | |
| rules { | |
| # Use a dynamic block to only generate the merge_queue configuration | |
| # if var.merge_queue is not null. | |
| dynamic "merge_queue" { | |
| for_each = var.merge_queue != null ? [var.merge_queue] : [] | |
| content { | |
| merge_method = merge_queue.value.merge_method | |
| min_entries_to_merge = merge_queue.value.min_entries_to_merge | |
| max_entries_to_merge = merge_queue.value.max_entries_to_merge | |
| check_response_timeout_minutes = merge_queue.value.check_response_timeout_minutes | |
| grouping_strategy = merge_queue.value.grouping_strategy | |
| } | |
| } | |
| } |
|
/hold |
2 participants
Summary
merge_queuevariable to thecommon_repositorymodule that creates agithub_repository_rulesetwith merge queue configurationstrictstatus checks on those repos since the merge queue handles testing against latest main automaticallyPrerequisites — merge these first
The E2E workflows in each repo need the
merge_grouptrigger before this PR is applied, otherwise the merge queue will time out (no workflow responds to merge group events) and block all merges.Do NOT merge this PR until all 5 above are merged.
Why merge queues
This is the GitHub Actions equivalent of Prow's
strict: true— but better. Instead of requiring authors to manually rebase when main moves ahead, the merge queue automatically creates a temporary merge ref on top of latest main, runs CI against it, and merges if it passes. No author action needed.Merge queue defaults
merge_methodmin_entries_to_mergemax_entries_to_mergecheck_response_timeout_minutesgrouping_strategyTest plan
tofu planshows 5 newgithub_repository_rulesetresources and 5 updatedgithub_branch_protectionresources (strict: true → false)🤖 Generated with Claude Code