fix(ci): resolve repo-root-relative git paths in OpenAPI base comparisons#335
Open
jeremi wants to merge 1 commit into
Open
fix(ci): resolve repo-root-relative git paths in OpenAPI base comparisons#335jeremi wants to merge 1 commit into
jeremi wants to merge 1 commit into
Conversation
…sons Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
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.
Summary
just openapi-contract <base_ref>checks (backed byproducts/notary/scripts/check-openapi-contract.shandcrates/registry-relay/scripts/check-openapi-contract.sh) use git's<rev>:<path>object syntax to fetch the OpenAPI document (and, forRelay, the reference config) at a base ref for an
oasdiff breakingcomparison.
<path>relative to the repo root, not thecurrent working directory. Both scripts
cdinto their productdirectory (
products/notary/crates/registry-relay) and then buildthe git object path from a directory-relative
SPEC_PATH(e.g.openapi/registry-notary.openapi.json), which git then looks up at therepo root, where it doesn't exist.
git cat-file -efails, and thescript's existing "handle bootstrap: file didn't exist at that ref yet"
branch quietly treats a path-resolution bug as though the file
never existed at the base ref, prints an innocuous-looking message,
and exits 0. No breaking-change diff ever runs.
Reproduction (before the fix)
From
products/notary, withorigin/mainfetched:but the file plainly exists at
origin/main:Same story for
crates/registry-relay(SPEC_PATHandREFERENCE_CONFIGboth mis-resolve;
just openapi-contract origin/mainprints the samefalse "did not exist... skipped" message and exits 0).
Fix
Minimal, path-resolution-only change to both scripts:
$ROOT-relative path viagit rev-parse --show-prefix/--show-toplevel, and self-check itwith
-efagainst the known-good local file before using it in anygit cat-file/git showcall. If the computed path doesn't resolveto the same file, the script now exits 1 with a clear
"failed to resolve repo-root-relative path... refusing to run base-ref
comparison" message instead of silently treating that as "file absent".
(legitimate bootstrap case, still exits 0 with the existing message)
distinguishable from "we couldn't resolve which file to even ask git
about" (now a loud, non-zero failure).
After the fix
target/openapi-contract/base.openapi.jsonis now actually written fromgit show, andoasdiff breakingruns (no differences vs. origin/main,since the branch only touches the two shell scripts). Same for
crates/registry-relay. The legitimate skip path was re-verified againsta real pre-existing-file base ref (the commit before
products/notary/openapi/registry-notary.openapi.jsonwas added) andstill exits 0 with the "did not exist... skipped" message, unchanged.
Verification
shellcheckon both changed scripts: clean.shfmt -i 4 -don both: no diff introduced by this change (the tworemaining reported lines are pre-existing
>vs>"..."redirectspacing the file already had before this change; left untouched to keep
the diff minimal and match surrounding style).
just openapi-contract <base_ref>andthe underlying scripts directly, as shown above.
repo-root-relative path) to confirm the new self-check fails loudly
(exit 1, clear message) rather than silently skipping.
Note (not part of this fix)
While investigating, I noticed CI (
.github/workflows/ci.yml) currentlyinvokes
just openapi-contractwith no base ref at all for eitherproduct, so today this code path is never exercised in CI — the bug was
latent rather than actively causing false-green PRs. Whether/how to wire
OPENAPI_CONTRACT_BASE_REF(or abase_refarg) into CI is a separate,larger decision (e.g. what to diff against for PRs vs. pushes to main)
and is out of scope here; flagging it so it can be tracked separately.
Found during review of PR #332; does not block it.
Test plan
shellcheckon both changed scriptsshfmt -i 4 -don both changed scripts (no new diff)via direct invocation and via
just openapi-contract origin/main)oasdiff breakingcomparisonagainst
origin/mainafter the fix (both scripts)skip path still works and still exits 0
instead of silently skipping