Skip to content

ci(release): gate tag on github, document the release process - #893

Merged
JarryShaw merged 1 commit into
mainfrom
fix-887-one-approval-release-gate
Sep 29, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix-887-one-approval-release-gate

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • fix
  • feat
  • perf
  • refactor
  • test
  • docs
  • ci
  • chore

Description of your pull request and other information

Closes #887.

Settings already dropped required_reviewers from conda-tag, pypi and anaconda, leaving one approval on github-release. This PR does the rest:

  1. tag depended on version_check alone, so with its own reviewer gone it would start before the one remaining approval and push a commit to main plus a conda-* tag. Now needs: [ github, version_check ]. Swept the job-level comments describing the old four-approval arrangement, including three stale rationales in tests/project/test_release_gates.py's own docstrings.
  2. Added tests/project/test_release_gates.py::TestGatedJobsDependOnGithub: every prior test in that module passes unchanged if tag's needs: is reverted to [ version_check ] (the exact regression ci(release): one approval for the whole release, not four separate environment gates #887 fixes), since none of them read needs:. The new test closes that gap.
  3. Added docs/source/releasing.rst: the only manual step is bumping pcapkit.__version__ (and only by hand if CITATION.cff moves with it, since the release path runs the full test suite and fails loudly otherwise). Documents both ways a release starts -- pushing a v* tag chooses the commit, not the version, since every tag/release name is built from pcapkit.__version__ and never from github.ref_name -- why one approval covers the whole pipeline, and precautions/recovery including the still-open fix(ci): a half-finished release leaves a v* tag that makes every retry skip silently #888 gap.

sphinx-build -b html: 58 warnings before and after (same set). python -m unittest tests.project.test_release_gates: 15 passed (confirmed the new test fails alone on the reverted needs:, and all 15 pass with it restored). Did not run the full suite.

@JarryShaw JarryShaw added ci Pull requests that change CI or workflow configuration (ci: subject prefix) docs Pull requests that change documentation only (docs: subject prefix) enhancement Issues requesting a new capability (set by the feature request template) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 29, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES on 1a55fe61c — cross-review (opus; author sonnet). The needs: change is right and the build/test figures reproduce exactly; three statements in releasing.rst are false, and one file the sweep should have reached was missed.

1. "The guard is moot by construction" on the tag-push path is wrong, and it is a mechanism error rather than wording. I measured it:

version_check checkout  (:108-111)  has NO `ref:`
PCAPKIT_VERSION         (:126-128)  python -c 'import pcapkit; print(pcapkit.__version__)'
tag-exists-action       (:132-135)  tag: "v${{ steps.get_version.outputs.PCAPKIT_VERSION }}"

The guard checks v<__version__ from the tree>, not the tag you pushed. Push v1.6.0 at a commit whose __version__ is 1.5.0b5 and it is asked about v1.5.0b5 — possibly already published — while startsWith short-circuits over the answer. push: tags: also fires on ref update, so a force-moved tag reaches that path having existed beforehand. And the document contradicts itself sixty lines later, where :150-155 describes the same path as risking "a double upload to an index that cannot take one back". The guard is unconditionally bypassed there; the protection is the operator's deliberate act, not an absence of risk. My own framing on #887 was closer, and the doc softened it.

2. "Editing __version__ by hand instead of running the script works too" is false — it blocks the release. Verified: tests/project/test_bump_version.py:493-513 asserts CITATION.cff's cited version equals pcapkit/__init__.py's __version__, its failure message ending "a hand-made version bump has to update both"; create-release.yml:92-99 calls unit-tests.yml with gate-only: true; version_check needs unit-tests. So the drift is loudly gated and stops the release before any approval is requested. bump_version.py's docstring says exactly this, and the doc cites that docstring as its authority while stating the opposite.

3. The tag name does not choose the version. Every downstream reference is v${{ needs.version_check.outputs.PCAPKIT_VERSION }}; nothing reads github.ref_name but the guard. A tag push picks the commit; __version__ in it picks the version. docs/source/pep.rst:888-889 already says "version-driven rather than tag-driven" — the new page contradicts existing docs.

4. The sweep missed tests/project/test_release_gates.py, which create-release.yml:85-87 cites as the assertion behind its own first bullet. Three passages still give four separate approvals as their rationale. Worse, and being fixed in the same round: that module never inspects needs: at all, so all 13 tests pass identically if tag's needs: is reverted — the exact regression #887 exists to prevent.

And a miss of my own, which the review caught: #888's body still recommended the hand-retag. I withdrew it in a comment but left the body saying it, so the issue and this doc disagreed in public. Body now corrected — struck through, with the reason it is both forbidden and non-functional.

Confirmed and needing no rework: the one-approval closure is necessary and sufficient (checked against live environment settings — github-release alone carries a reviewer); conda-tag writes to main as described; the #888 characterisation is accurate and correctly refuses the hand-retag; the unmeasured-claim flag at :149-151 is honest and the argument holds either way; sphinxcontrib.mermaid is at conf.py:81 with the block actually rendering; both internal references resolve; and the build is 58 warnings before and after, byte-identical sets. The diagram's "matrix: 2 OS" is right and corrects the 3-OS figure I gave on #887.

Three non-blocking items also sent back: the OIDC claim says a missing environment: pypi would break the upload where can is supportable from here; "the only step a person performs" omits the unconditional changelog job and the undated-heading warning; and the diagram omits the direct version_check edges, fine as a transitive reduction if it says so.

`required_reviewers` was already removed from `conda-tag`, `pypi` and
`anaconda` (settings only, done outside this change), leaving one
approval on `github-release`. `tag` still depended on `version_check`
alone, so it would start the moment that job finished -- before the
one remaining approval -- and push a commit to `main` plus a
`conda-*` tag. Fix: `needs: [ github, version_check ]`.

Swept the job-level comments (above `jobs:`, and on `github`, `tag`,
`pypi`, `conda`) that described the old four-approval, two-wave
arrangement, including three stale rationales in
`tests/project/test_release_gates.py`'s own docstrings.

Added `tests/project/test_release_gates.py::TestGatedJobsDependOnGithub`:
every existing test in that module passes unchanged if `tag`'s
`needs:` is reverted to `[ version_check ]` -- the exact regression
#887 fixes -- because none of them read `needs:` at all. The new test
asserts every gated job's `needs:` closure includes `github`.

Added `docs/source/releasing.rst`: the only manual step is bumping
`pcapkit.__version__`, and by hand only if `CITATION.cff` moves with
it in the same commit, since the release path's own test gate runs
the full suite and fails loudly otherwise. Documents both ways a
release starts -- pushing a `v*` tag chooses the commit, not the
version, since every tag/release name is built from
`pcapkit.__version__` and never from `github.ref_name` -- why one
approval on `github-release` covers the whole pipeline, and
precautions/recovery including the still-open #888 silent-skip gap.
Registered in `docs/source/index.rst`.

Build: `sphinx-build -b html` -- 58 warnings before and after, same
set. `python -m unittest tests.project.test_release_gates` -- 15
passed (confirmed the new test fails alone if the `needs:` fix is
reverted, and that all 15 pass with it restored).

Closes #887
@JarryShaw
JarryShaw force-pushed the fix-887-one-approval-release-gate branch from 1a55fe6 to cc14e8a Compare September 29, 2026 01:46
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO on cc14e8ac3 — all four blocking items fixed, verified against source rather than from the report.

The gate hole is genuinely closed, and I reproduced it. Patched tag's needs: back to [ version_check ] on the PR head and re-ran the suite:

as-shipped         : run=15 failures=0 errors=0
with needs reverted: run=15 failures=1 errors=0
  X test_every_gated_job_depends_on_github (TestGatedJobsDependOnGithub) (job='tag')

Exactly one failure, and it is the new test, naming the offending job. Before this PR all 13 tests passed with the regression present — so the file now asserts the half of the gate that moved from environment: to needs:. That is the difference between a test suite that documents the arrangement and one that defends it.

The three corrections are in and the old wording is gone — "moot by construction" now appears zero times, "unconditionally bypassed" twice (the tag-push bullet and the hand-retag paragraph, which carried the same error):

  • The guard now says its PCAPKIT_TAG_EXISTS half "can never carry the run, because startsWith alone already satisfied the ||", that nothing checks whether the tagged commit's version was already published, and that a force-moved tag takes the same path — the ref-update case I had missed too.
  • Hand-editing __version__ now says it "does not drift through silently, it blocks the release", citing the citation test and the gate-only: true path that makes it non-optional, with the changelog job and undated-heading warning added right after so "the only manual step" is no longer unqualified.
  • The tag name now says it "chooses which commit to release, not which version", with the dangling-v1.6.0 worked example and a cross-reference to pep's "version-driven rather than tag-driven" — so the new page agrees with the existing one instead of contradicting it.

The author also found corroboration I had not asked for: pypi and conda check out with ref: v${{ … PCAPKIT_VERSION }} and ref: conda-${{ … }}+0 at :338/:424 — version-derived, never github.ref_name — which independently confirms the mechanism.

Every line citation was re-derived after the comment sweep shifted the file by +4, which is the kind of thing that silently rots: tag's needs: is :251 now, not :247; pypi :319; conda :406. I verified those three directly.

Also fixed: three stale four-approval rationales in tests/project/test_release_gates.py, the OIDC claim softened to can with an explicit note that PyPI-side publisher configuration was not read, and the Mermaid diagram labelled as a transitive reduction. Build unchanged at 58 warnings with a byte-identical set; test_release_gates.py 15/15; test_bump_version.py 47/47 untouched.

Unpublished and unmerged, yours to merge — it closes #887, and unblocks #888.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 29, 2026
@JarryShaw
JarryShaw merged commit 486c2a2 into main Sep 29, 2026
63 checks passed
@JarryShaw
JarryShaw deleted the fix-887-one-approval-release-gate branch September 29, 2026 02:07
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 29, 2026
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Pull requests that change CI or workflow configuration (ci: subject prefix) docs Pull requests that change documentation only (docs: subject prefix) enhancement Issues requesting a new capability (set by the feature request template)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

ci(release): one approval for the whole release, not four separate environment gates

1 participant