ci: scope a pull request's conformance to the cases it changed - #176
Merged
Conversation
Conformance did not run on pull requests at all: discover-tests was gated on `github.event_name != 'pull_request'`, so a new or edited fixture stayed unverified until it reached main and the only early check was dispatching debug-conformance.yml by hand with the case names typed out. ci.yml now resolves its scope from the event via scripts/ci/conformance-scope.sh: push / workflow_dispatch -> every case in testdata/ (117) pull request -> only the cases the PR touched pull request + full-conformance -> every case in testdata/ A PR touching no fixture resolves to zero and skips conformance-tests, pre-cleanup and post-cleanup entirely, so a plumbing PR stops paying for 117 live GCP lifecycles. Mapping rules: added/copied/modified/renamed testdata/*.pkl are in scope; deletions are ignored; an -update / -replace companion maps back to its case; testdata/config/ is shared Pkl, not a fixture (git's pathspec glob matches '/', so it has to be filtered out explicitly); a changed companion with no base fixture fails the run rather than filtering to nothing and passing green. .github/conformance-pr-skip.txt is optional and absent today - names in it are subtracted from a PR's scope, and a name in it that matches no fixture fails the run so the two cannot drift. conformance-scope_test.sh covers all of this against throwaway git repos with no cloud credentials, and runs in the test-unit job so it cannot rot.
bucket-update.pkl carried the comment "This is the mutable field being tested (changed from false to true)" next to `enabled = false` - the same value as bucket.pkl. The update phase therefore only ever changed a label, and the versioning path it claimed to cover was never exercised. `versioning` is a direct field in bucket_body_builder.go, so the PATCH already carries it; the fixture just never asked for a different value.
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.
Problem
Conformance doesn't run on pull requests.
discover-testsis gated ongithub.event_name != 'pull_request', which skips the matrix,pre-cleanupandpost-cleanupwith it. A new or edited fixture is unverified until it reachesmain, and the only early check is hand-dispatchingdebug-conformance.ymlwith the case names typed out.What changes
ci.ymlresolves its conformance scope from the event:main/workflow_dispatchtestdata/(117)full-conformancelabeltestdata/A PR touching no fixture resolves to zero and skips
conformance-tests,pre-cleanupandpost-cleanupentirely — a plumbing or refactor PR stops paying for 117 live GCP lifecycles.Unlike the Azure sibling (formae-plugin-azure#129), there is no curated matrix file here: full scope stays the
testdata/discoveryci.ymlalready did, moved into the script unchanged.Mapping rules
In
scripts/ci/conformance-scope.sh:testdata/*.pklare in scope. Deletions ignored — nothing left to run.-update/-replacecompanion maps back to its case, since one matrix entry drives the whole lifecycle. Editing onlybucket-update.pklstill runsbucket— which is exactly what this PR does to itself, see Verification.testdata/config/is shared Pkl, not fixtures. Git's pathspec glob matches/, so-- 'testdata/*.pkl'also catchestestdata/config/vars.pkl; without an explicit top-level-only filter it would resolve to a bogus case namedconfig/varsand fail the run. Filtered, with a test..github/conformance-pr-skip.txt— mechanism only, no fileNames in this file are subtracted from a PR's scope, for cases that cannot pass in CI regardless of the diff. The file is not in this PR. The AlloyDB and VPN-gateway families are the obvious candidates on cost and runtime, but they run on push today and I have no evidence they are PR-unrunnable, so seeding them would be a claim I can't back. Add lines when a case proves it.
A name in the file matching no fixture fails the run, so the two can't drift into disagreement. The file does not affect push or nightly.
Known gap
A change under
pkg/ortestdata/config/vars.pklaffects every resource but touches no fixture, so it resolves to an empty scope. Two backstops: label the PRfull-conformance, or rely on push-to-main, which still runs everything. Documented inCONTRIBUTING.mdrather than auto-escalated — escalating a one-linevars.pkledit to 117 live cases costs more than it catches.Verification
scripts/ci/conformance-scope_test.sh— 13 cases against throwaway git repos, no cloud creds, wired into thetest-unitjob so it can't rot:Resolved against real history too. Replaying
6ad92ea("20 more resource types") as if it were a PR reproduces its fixture set rather than the whole matrix:This PR is its own smoke test. The second commit edits
testdata/bucket-update.pkland nothing else undertestdata/, so the companion→base rule is what putsbucket— and onlybucket— on the matrix. If the CI run below shows one conformance job namedbucket, the mechanism works end to end.That second commit is also a real fixture fix:
bucket-update.pklcarried the comment "This is the mutable field being tested (changed from false to true)" next toenabled = false, the same value asbucket.pkl. The update phase only ever changed a label; the versioning path it claimed to cover was never exercised.versioningis a direct field inbucket_body_builder.go, so the PATCH already carries it — the fixture just never asked for a different value.ci.ymlparses;reuse lintclean (588/588).Notes for review
fetch-depthternary trap.${{ event_name == 'pull_request' && 0 || 1 }}evaluates to1on a pull request, because0is falsy in a GitHub expression and||then takes the right branch. Hardcoded0with a comment saying why.if: needs.discover-tests.outputs.count != '0'onconformance-testsandpre-cleanupis what makes "this PR touched no fixture" green instead of red.gcp-conformance-testsbecause they target one project. GitHub keeps only one run queued per group, so a third concurrent run evicts the waiting one and it reportscancelled, not failed. PR-triggered conformance makes that more likely. Documented inCONTRIBUTING.md; splitting the cheap checks out of the group is a separate change.pull_requestpath.