Skip to content

Commit 32744d8

Browse files
ZIJZIJ
committed
Agent task to allow apply when only apply blocks mergeability
1 parent d81c3ea commit 32744d8

File tree

1 file changed

+106
-0
lines changed

1 file changed

+106
-0
lines changed

‎agent-tasks/allow-apply-when-only-apply-blocks-mergeability.md‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
# Allow Apply When Only `digger/apply` Blocks Mergeability
2+
3+
- Related issues: #1150, #959
4+
- Areas: `libs/ci/github`, `cli/pkg/digger`, `backend/controllers`, `docs`
5+
6+
## Goal
7+
Unblock `digger apply` when GitHub reports PR mergeability state as "blocked" but the only blocking status checks are Digger’s own aggregate/per‑project apply checks (`digger/apply` and `*/apply`).
8+
9+
## Problem / Context
10+
- We gate apply on mergeability: `IsMergeable()` must be true, otherwise `digger apply` errors: "cannot perform Apply since the PR is not currently mergeable".
11+
- When `digger/apply` is configured as a required status, GitHub reports the PR as `mergeable=false` with `mergeable_state=blocked` until apply completes.
12+
- This creates a circular dependency: apply is needed to make the PR mergeable, but apply refuses to run because the PR is not mergeable.
13+
14+
Current code (GitHub):
15+
- `IsMergeable(prNumber)` returns `pr.GetMergeable() && isMergeableState(pr.GetMergeableState())` where allowed states are `clean`, `unstable`, `has_hooks`.
16+
- Apply gate uses this in two places:
17+
- `cli/pkg/digger/digger.go` before running apply.
18+
- `libs/apply_requirements.CheckApplyRequirements` when `apply_requirements` includes `mergeable` (default).
19+
20+
## Non-Goals
21+
- Changing auto-merge behavior after successful applies (we already require combined status `success`).
22+
- Implementing Checks API inspection (we only need to introspect commit statuses we set today).
23+
- Changing mergeability logic for non-GitHub providers.
24+
25+
## Approach
26+
Augment GitHub mergeability logic to handle the chicken‑and‑egg case:
27+
28+
1) Keep existing fast path: if REST `mergeable==true` and `mergeable_state` in {`clean`, `unstable`, `has_hooks`}, return true.
29+
2) If `mergeable_state == "blocked"`, fetch combined commit statuses for the PR head SHA.
30+
3) If every non‑success context is either:
31+
- `digger/apply` (aggregate), or
32+
- per‑project apply contexts matching `*/apply` (e.g., `projectA/apply`),
33+
then treat the PR as mergeable for the purpose of running apply. Otherwise, return false.
34+
4) Log diagnostics: mergeable bool, mergeable_state, and the list of non‑success contexts checked.
35+
36+
Notes
37+
- Reviews/draft can also yield `blocked` but won’t appear in commit statuses. If users want to require approvals, they should include `approved` in `apply_requirements` (separate gate already exists). We deliberately do not block here based on reviews unless configured via `apply_requirements`.
38+
- We continue to respect `skip_merge_check` and project-level `apply_requirements` semantics.
39+
40+
## Acceptance Criteria
41+
- When a PR is `mergeable_state=blocked` and the only non‑success statuses are `digger/apply` and `*/apply`, `digger apply` proceeds (no "not currently mergeable" error).
42+
- If any other non‑success status exists (e.g., tests, lint, `digger/plan`, etc.), `digger apply` remains blocked.
43+
- Auto-merge flow is unchanged and still requires combined status `success`.
44+
- Logs include helpful diagnostics showing the evaluated mergeable state and non‑success contexts.
45+
- Documentation reflects the new behavior and updates the known-issues note.
46+
47+
## Implementation Plan
48+
49+
1) Add pure evaluator (unit-testable)
50+
- File: `libs/ci/github/github.go`
51+
- Function: `isMergeableForApply(mergeable bool, mergeableState string, nonSuccessContexts []string) bool`
52+
- Return true if `(mergeable && isMergeableState(mergeableState))` OR `(mergeableState == "blocked" && all nonSuccessContexts ∈ {"digger/apply" ∪ contexts with suffix "/apply"})`.
53+
- Treat statuses with state in {`pending`, `failure`, `error`} as non‑success.
54+
55+
2) Gather non‑success contexts from GitHub
56+
- In `GithubService.IsMergeable(prNumber)`:
57+
- Fetch PR via REST (already done).
58+
- Fetch combined statuses for the PR head SHA (we already have `GetCombinedPullRequestStatus`; add a sibling helper to return raw contexts and their states).
59+
- Build `nonSuccessContexts` from the combined statuses where state != `success`.
60+
- Return `isMergeableForApply(pr.GetMergeable(), pr.GetMergeableState(), nonSuccessContexts)`.
61+
- Add `slog.Debug` with fields: `mergeable`, `mergeableState`, and `nonSuccessContexts`.
62+
63+
3) Wire through existing gates (no interface changes)
64+
- `cli/pkg/digger/digger.go`: keep using `prService.IsMergeable()` (behavior now includes the new allowance under GitHub).
65+
- `libs/apply_requirements/apply_requirements.go`: no changes, it will inherit new behavior.
66+
67+
4) Tests
68+
- Add unit tests for `isMergeableForApply` (table‑driven) covering:
69+
- mergeable true + clean/unstable
70+
- blocked + only `digger/apply`
71+
- blocked + `projectA/apply` and `projectB/apply`
72+
- blocked + additional failing context (e.g., `ci/test`) → false
73+
- non‑blocked + mergeable=false → false
74+
- Optional: add an integration-ish test for `GithubService.IsMergeable` using a small seam to inject combined status data (or validate via a thin wrapper if direct client mocking is hard).
75+
76+
5) Docs
77+
- Update `docs/ce/howto/apply-requirements.mdx`:
78+
- Adjust the note on mergeability to state that Digger now proceeds with apply if the only blocker is Digger’s own apply status.
79+
- Keep guidance about avoiding circular required checks for other providers or check-run based integrations.
80+
81+
## Code Pointers
82+
- GitHub mergeability and statuses:
83+
- `libs/ci/github/github.go`: `IsMergeable`, `GetCombinedPullRequestStatus`, `isMergeableState`
84+
- Apply gates:
85+
- `cli/pkg/digger/digger.go`: mergeability check before apply
86+
- `libs/apply_requirements/apply_requirements.go`: mergeable requirement
87+
- Status producers:
88+
- `backend/utils/github.go`: sets aggregate `digger/plan` and `digger/apply`
89+
- per‑project statuses: same file via `job.GetProjectAlias()+"/apply"`
90+
91+
## Risks & Mitigations
92+
- Missed non‑success contexts (Checks API): We only inspect commit statuses; if users require check runs (not statuses), behavior remains unchanged. Document this and consider future enhancement for Checks API.
93+
- Over‑permissive in rare cases: If other blockers do not surface as statuses (e.g., code owners, required reviews) and users don’t add `approved` requirement, apply might run despite reviews missing. This aligns with current semantics and can be controlled via `apply_requirements`.
94+
95+
## Rollout
96+
- Feature is effectively on by default via updated `IsMergeable` logic for GitHub only.
97+
- Observe logs for `nonSuccessContexts` on real repos; rollback by reverting the helper usage if needed.
98+
99+
## Verification Steps
100+
- Scenario A: Only `digger/apply` pending
101+
- Observe `mergeable_state=blocked`; statuses show only `digger/apply` (and/or `*/apply`) pending → `digger apply` runs.
102+
- Scenario B: Additional pending status (e.g., tests)
103+
- With `ci/test` pending → `digger apply` remains blocked.
104+
- Scenario C: Clean/unstable
105+
- `mergeable=true`, `state=clean|unstable` → unchanged, `digger apply` runs.
106+

0 commit comments

Comments
 (0)