fix(check-for-epoch-bump): compare against origin/main, not local main - #37
Open
AmberArcadia wants to merge 2 commits into
Open
fix(check-for-epoch-bump): compare against origin/main, not local main#37AmberArcadia wants to merge 2 commits into
AmberArcadia wants to merge 2 commits into
Conversation
The hook read its baseline with `git show main:<file>`, which is the local `main`
branch. Package repos tell contributors never to commit to main, so that branch
is only as fresh as the last time someone checked it out. Mine in
chainguard-dev/stereo is far enough behind that
`git show main:enterprise-packages/gitlab-cng-19.0.yaml` returns
`fatal: path ... exists on disk, but not in 'main'`.
When that `git show` failed the script fell through to `version_main=0` and
`epoch_main=0`, so every package compared as increased. On a real stereo file the
old code printed:
✅ Version has been increased compared to main: 19.0.5 > 0
for a package that has been on main for months and is at 19.0.5-r2 there. The
`> 0` was the only hint that no comparison had happened. With the fix, the same
file reports the truth:
⚠️ Epoch HAS NOT been increased compared to origin/main: 0 <= 2 (version: 19.0.5)
A stale local main is the quieter half of the bug. While the branch still
contains the file but lags a few epochs, the baseline is simply too low, so an
epoch that is already taken on main passes as a fresh bump.
Changes:
- Resolve the baseline once, preferring origin/main, then main, then origin/HEAD.
No `git fetch`: a pre-commit hook has to stay fast and work offline, and the
last fetched origin/main still beats a branch nobody visits.
- Separate "cannot resolve any base ref" from "file is absent from the base ref".
The first now says so and skips, instead of silently passing everything. The
second keeps the 0-r0 baseline, which is correct for a new package, but prints
`ℹ️ Not found on <ref> (new package?)` so a real comparison is not implied.
- Restore the missing-field warnings the version/epoch rewrite dropped. A yaml
with no top-level `version:` or `epoch:` said nothing and silently became 0.
- Name the ref in every verdict line, so what was compared is visible.
- Drop a redundant `version_local != version_main` re-test inside the branch that
had already established it.
Exit status is unchanged: still always 0, matching `verbose: true` and the hook's
advisory role. This only changes which baseline is used and what gets reported.
Adds tests/test-check-for-epoch-bump.sh, which builds throwaway repos with a real
origin remote so local main and origin/main can disagree. It covers a local main
that is one commit behind, a local main predating the file, no staleness at all,
a genuinely new package, and a repo with no resolvable base ref. Five of its ten
cases fail against the previous script and all ten pass now. The scratch repos set
commit.gpgsign=false: gitsign is enabled globally on dev machines, so without
that every throwaway commit round-trips to Fulcio and Rekor.
Nothing was exercising either. The new tests/ directory would have sat unrun, and this repo ships shell scripts as its product yet the Lint job's pre-commit config had no shellcheck hook. Adds a `Shell script tests` job that runs tests/test-check-for-epoch-bump.sh. Verified it works under a bare environment, `env -i HOME=/tmp/fakehome PATH=/usr/bin:/bin`, which is the relevant CI condition: the tests build their own throwaway repos with their own committer identity, so they need nothing from the runner's git config. 10 of 10 pass there. Adds the shellcheck hook from shellcheck-py, frozen at v0.11.0.1, matching the existing `rev: <sha> # frozen: <tag>` convention. Picked shellcheck-py over the Docker-based koalaman mirror so it installs alongside the pip packages the Lint job already sets up, with no daemon needed. No args: shellcheck's default severity is already `style`, and I confirmed the output is identical with and without an explicit `-S style`. All three scripts in the repo, including the pre-existing scripts/check-restored-packages.sh, are clean: shellcheck 0.11.0 exits 0 with zero findings, so this adds no backlog to work through. Full `pre-commit run --all-files` passes locally with the new hook, and `actionlint` exits 0 on the modified workflow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
check-for-epoch-bumpread its baseline withgit show main:<file>, using the localmainbranch. This switches it to preferorigin/main, and makes the two "no baseline" cases distinguishable instead of both collapsing into a silent pass.Why
Package repos tell contributors never to commit to main, so local
mainis only as fresh as the last time someone happened to check it out. In mychainguard-dev/stereoclone it is stale enough that the file lookup fails outright:When that
git showfailed, the script fell through toversion_main=0/epoch_main=0, and every package compared as increased. Same file, before and after:gitlab-cng-19.0has been on main for months and is at19.0.5-r2there. The> 0was the only clue that no comparison had taken place, and it is easy to read as a pass. I hit this while resolving a batch of stereo version-bump conflicts: the hook reported green for every package I touched, so it contributed nothing.The stale-but-present case is the quieter half. While local
mainstill has the file but lags a few epochs, the baseline is just too low, so an epoch already taken on main passes as a fresh bump. That is the failure that actually costs someone a rebuild.Changes
origin/main, thenmain, thenorigin/HEAD. Nogit fetchis added: a pre-commit hook has to stay fast and work offline, and the last fetchedorigin/mainstill beats a branch nobody visits.0-r0baseline, which is right for a new package, but printsℹ️ Not found on <ref> (new package?)so it does not masquerade as a real comparison.version:orepoch:silently became0.version_local != version_mainre-test inside the branch that had already established it.Exit status is unchanged: still always
0, matchingverbose: trueand the hook's advisory role. This PR changes which baseline is used and what gets reported, not whether commits are blocked.Tests
Adds
tests/test-check-for-epoch-bump.sh. It builds throwaway repos with a realoriginremote so localmainandorigin/maincan actually disagree, and asserts the harness set that divergence up before trusting a green result.Cases: local
mainone commit behind, localmainpredating the file, no staleness at all, a genuinely new package, and a repo with no resolvable base ref.The five that fail on the old script are exactly the reported defects.
bash -nis clean on both files, andshellcheck -S stylereports no findings on either (exit 0, run via thekoalaman/shellcheck:stableimage since shellcheck is not installed on my machine). Worth noting theLintjob's pre-commit config here does not include shellcheck, so CI does not cover it.One note for anyone extending the tests: the scratch repos set
commit.gpgsign=false. gitsign is enabled globally on dev machines here (gpg.format=x509), so without that every throwaway commit round-trips to Fulcio and writes into the Rekor log.Making the tests and the scripts actually checked
Second commit,
ci:. Neither of these existed before, so the newtests/directory would have sat unrun and the shell scripts this repo ships as its product were unlinted.Shell script testsjob runstests/test-check-for-epoch-bump.sh. Verified underenv -i HOME=/tmp/fakehome PATH=/usr/bin:/bin, which is the condition that matters in CI: the tests build their own repos with their own committer identity, so they need nothing from the runner's git config. 10 of 10 pass there.shellcheckhook fromshellcheck-py, frozen atv0.11.0.1, following the existingrev: <sha> # frozen: <tag>convention. I pickedshellcheck-pyover the Docker-basedkoalamanmirror so it installs alongside the pip packages theLintjob already sets up. No args, because shellcheck's default severity is alreadystyleand I confirmed the output is byte-identical with and without an explicit-S style.All three scripts in the repo,
check-for-epoch-bump.sh, the pre-existingcheck-restored-packages.sh, and the new test, are clean: shellcheck 0.11.0 exits 0 with zero findings, so this adds no backlog.pre-commit run --all-filespasses locally with the new hook, andactionlintexits 0 on the modified workflow.Consumer impact
Repos pinning an older
revare unaffected until they bump. On bumping, expect previously-silent✅ ... > 0lines to turn into real verdicts, some of them⚠️. Those warnings are accurate and were being suppressed; since the hook does not fail, nothing starts blocking.