Add conformance scenarios for the observer layer (retries, GitHubEvent triggers, sign-up) - #45
Merged
Merged
Conversation
trigger mirroring, and Sensor-confirmed sign-up New tests/fake/observers suite drives build-observer, buildrun-observer, and githubevent-observer directly (via new pkg/controller/observers/* aliases in environments-controller — companion PR blanketops/environments-controller#138). 15 specs, all real, all passing: - build-observer.applyTriggers: mirrors a matching GitHubEvent's commit metadata onto Build annotations, skips non-matching event types, is idempotent on an unchanged SHA. - build-observer.applyRetry: bumps retry-attempt on a failed terminal BuildRun with attempts remaining, stops at MaxAttempts, clears on success, no-ops when the retry policy has onFailure disabled — plus one scenario chaining a real buildrun-observer write into build-observer's read, not hand-set status. - buildrun-observer: writes BuildSuccess/BuildFailed conditions and the status contract (ExecutionRef, Success) from a BuildRun's terminal Succeeded condition; ignores non-terminal runs and unlabeled BuildRuns. - githubevent-observer: the actual "signed up" confirmation — stays quiet with no payload, GitHubEventReceiving once a payload arrives but the Sensor hasn't confirmed, GitHubEventReady once it has. Writing these surfaced three real bugs fixed at the source, not worked around here: - resolve.go read the Build policy's raw contract at "triggers" instead of "allowedTriggers" (the actual proto field name) — trigger matching was always empty in production (blanketops/environments#324). - StatusWriter.Write silently dropped Status.Contract entirely — only conditions ever persisted, so build-observer's applyRetry was reading a field nothing ever wrote (same PR). - This repo's own TriggerType fixture constants were capitalized ("Push") while the real system's normalizeEventType always lowercases before comparing — fixed here, only used in this one fixture. Also extends GitHubEventContract with eventId/commitSHA/actor — fields the real resolver already reads from spec.contract that the fixture never carried, so no fixture GitHubEvent could ever represent a real received webhook payload. CI: run-tests.yml now also installs the Shipwright BuildRun and Argo Events Sensor CRDs (not their controllers) the new suite needs, pulled from the already-resolved Go module cache rather than a separate fetch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI failed intermittently: tests/grpc's ListBuilds scans "default"
unscoped, and go test runs packages concurrently against the same
shared cluster by default (no -p 1 anywhere in this repo's CI). This
package's Build fixtures use the plain-JSON contract shape (matching
what resolve.go expects), not the protojson shape ListBuilds' fake
server expects when it unmarshals every Build it finds — so a Build of
ours caught mid-flight in "default" broke ListBuilds with a protojson
parse error ("unexpected token \"push\""). The applyRetry MaxAttempts
scenario's ~3s of deliberate sleeps (for a deterministic BuildRun
ordering) widened that window enough to hit in CI reliably, confirmed
locally as timing-dependent — same code passed some local runs and
failed others depending on scheduling.
Moved this package's Build/BuildRun fixtures to a dedicated
"observers-test" namespace instead of chasing the timing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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
Adds
tests/fake/observers— 15 real, passing scenarios coveringbuild-observer,buildrun-observer, andgithubevent-observer(the side-channel reconcilers underenvironments-controller/internal/controller/observers, previously untested anywhere). Directly answers: how do we stand on retries, GitHubEvent-driven Build trigger mirroring, and GitHubEvent sign-up confirmation.Depends on two companion PRs, currently referenced via pseudo-version in
go.mod:pkg/controller/observers/*aliases (they lived only underinternal/, unreachable from this repo without it)allowedTriggersJSON-key bug and theStatusWriter.Writebug (see below)Once those merge and cut real releases,
go.modhere should be bumped from the pseudo-versions to the tagged versions.What writing these scenarios actually found
Three real production bugs, all fixed at the source (not worked around in fixtures):
resolve.goread the Build policy's raw contract at key"triggers"instead of the proto's actualallowedTriggers— trigger matching was silently always empty for any real Build.StatusWriter.Writenever persistedStatus.Contract, only conditions —buildrun-observersets it expecting persistence (its own doc says so), but it silently never landed. Sincebuild-observer.applyRetrydecides retries by reading that field, the retry mechanism was dead in production, not just untested.TriggerTypefixture constants were capitalized ("Push") while the real system'snormalizeEventTypealways lowercases before comparing — fixed here.Also extended
GitHubEventContractwitheventId/commitSHA/actor— fields the real resolver already reads that the fixture never carried, so no fixture GitHubEvent could ever represent a real received webhook.Test plan
go test ./...— all green, 15/15 new specs pass plus no regressions anywhere elsebuildrun-observerwrite intobuild-observer's read (not hand-set status) — proves the full handoff, the exact thing bug Test #2 brokerun-tests.yml) updated to install the ShipwrightBuildRunand Argo EventsSensorCRDs the new suite needs (CRDs only, not their controllers — same pattern as the rest of this repo)🤖 Generated with Claude Code