muditagrawal2007 commented on PR #74909:
URL: https://github.com/apache/airflow/pull/74909#issuecomment-6238571514
Thanks for the careful layering here — stacking on #74825 keeps the diff
focused, and the "post the closing comment first" fix is a real correctness
improvement that was hard to notice in the previous PR. Reading the design
across the five follow-ups: the protect-the-gate guard closes the same
self-deciding hole that motivated the original gate, the shared-head handling
makes a subtle invariant explicit, and the documented ambiguity in #74905 /
#74906 is now resolved by code rather than left as future work.
A few observations, mostly small and offered as questions rather than
blockers:
### 1. The `gate_modified` marker and `gate_checked`
The other closure paths — `gate_bypassed`, the inactivity-close, the regular
state branches — all now write `Marker(..., gate_checked=gate_checked)`. The
new `gate_modified` branch writes `Marker(ForkState.GATE_MODIFIED.value,
pr.head_sha, now)` without it. The doc says:
> The state comment's marker records `"gate_checked": true` with the head
commit only when this check ran for it
The check *did* run for the head commit, so on a literal reading the flag
should be set. The reason it isn't is the maintainer-reopen-without-restore
path: `already_checked` depends on `marker.gate_checked` being False so the
next reconcile re-runs the check and closes again. So the code is internally
consistent, but the doc doesn't capture that exception.
Two ways it could read better, whichever you prefer:
- Mirror the doc to the code: always write `gate_checked=gate_checked`, and
extend `already_checked` with `pr.last_reopened_at is None or
pr.last_reopened_at <= marker.since`. Same behaviour, no doc exception.
- Or add a note next to that doc sentence saying the gate-modified close
path keeps `gate_checked` unset so a maintainer reopen re-runs the check.
Whichever direction you take, `test_a_change_to_the_gate_closes` only
asserts on `"state": "gate_modified"` in the marker, so the current choice is
invisible to CI. Tightening to `'"state": "gate_modified"' in
plan.marker_comment and "gate_checked" not in plan.marker_comment` would lock
it in either direction.
### 2. `classify` reads as several branches stacked together
After this PR it does, in order: `use project ci` short-circuit, ungated
short-circuit, gate tampering, gate-modified tamper, reopen-since-closing, fork
state observation, shared-head decision, draft/labels, status, marker. Each
branch is short on its own. Nothing is broken — it's just that as a single
~270-line function it's harder to follow than it deserves to be when five
follow-ups land in the same diff. A small refactor (e.g. `_plan_closure(...)`
for the three close paths and `_plan_record_state(state, since)` for the three
record-only paths, with `classify` as a thin orchestrator) would keep the
existing `Plan` shape and tests intact while making future reviews easier.
### 3. `observe_fork_state` has no `gate_modified` case
```python
if fork.workflow_state is None:
if marker is not None and marker.state == ForkState.AWAITING_PUSH.value:
return ForkState.AWAITING_PUSH, None
return None, "no fork run and the fork workflow state was not read"
```
For a re-opened `gate_modified` PR with no fork run, this returns `(None,
"no fork run...")` rather than `(GATE_MODIFIED, None)`. Tolerant today because
the gate-modified check runs above `observe_fork_state`. If
`observe_fork_state` becomes the source of truth for that state later, an
explicit case would be needed — otherwise we'd start emitting `unknown` lines
per run.
### 4. `_other_pull_requests` paginated once per `find()` call
Result depends only on `(head_repo, head_ref, window_start)` and `find()` is
invoked once per branch. Hoisting it into `BypassFinder.__init__` (computed
once with the same six-hour window) would save one paginated call per PR per
reconcile. Small thing — visible mainly in the `GitHub calls: {...}` log line.
### 5. `_set_status` URL consistency
The new branch hardcodes
`f"https://github.com/{repo}/pull/{pr.number}/files"` while the other paths use
`fork.run_url or fork_actions_url(pr.head_repo)` or
`f"https://github.com/{repo}/pull/{pr.number}/checks"`. The files URL is the
right target semantically — a `gate_modified` PR has no fork run to link — but
a `gate_changed_files_url(pr.number)` helper would keep call sites uniform and
easy to grep.
### 6. `_list_jobs` fail-open path
```python
try:
jobs = self._list_jobs(run)
except GitHubError:
result = True
```
On first read, "no jobs listed → counts as project CI" sounds backwards. The
intent (better to flag a run that was CI than miss one) is correct, but the
intent isn't captured in a comment. The existing
`test_finder_catches_triggers_the_base_branch_does_not_have` was updated to
provide non-empty jobs, which pins the False path; a small test for the raise
path would round it out and stop a future refactor from flipping the fail
direction.
### 7. `base_at` boundary
```python
for changed_at, previous, _ in reversed(pr.base_changes):
if when < changed_at:
base = previous
```
A run whose `created_at` equals the retarget `createdAt` applies the new
base; a run a millisecond earlier applies the old base. The asymmetry is
intentional and reads sensibly, but a one-line comment ("runs at or after a
retarget are against the new base") would help the next reader who hits the
strict-less-than.
### 8. `GATE_PROTECTED_PATHS` and the four back-references
It's one frozenset at module scope plus four "this file is part of the gate"
header comments. Adding a new gated file requires updating the set *and* all
four comments. Optional, but a small test that pulls those header strings and
asserts they reference entries in `GATE_PROTECTED_PATHS` would catch a
forgotten update before someone changed a gate file and slipped past the check.
### 9. Minor
- `Marker(ForkState.GATE_MODIFIED.value, pr.head_sha, now)` and the
state-string literal could share a const — at this size it's fine to leave
inline.
- `from dataclasses import replace` is used in `is_own_pilot_change` exactly
as expected; flagging only so it isn't read as a stray import.
- The "Gate protection" section in
`08_fork_ci_for_external_contributors.md`, the table-row addition, and the
"**New comments go first.**" rule are all genuinely good changes — reading
those gave the rest of the diff a lot of context.
Overall the design holds together: the gate-tampering and shared-head fixes
map cleanly onto #74905 / #74906 / #74907, the test density matches the risk of
each new branch, and the run-time invariants (once per head commit, exactly one
closure comment, retarget resets trust) all hold. The only thing I'd genuinely
want resolved before merge is the `gate_modified` marker flag alignment in #1 —
whichever way you settle it, a one-line comment at the construction site would
document the choice for whoever reads it next.
Happy to expand any of these into a separate PR or thread if that'd be more
comfortable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
