feat(revision): version-agnostic revision.Manager#1145
Open
craigmarker wants to merge 12 commits into
Open
Conversation
Go Coverage Report (Bazel)Total Coverage: 63.8% Coverage Policy:
|
craigmarker
force-pushed
the
craig/revision-schema
branch
from
ebcf5a2 to
d61af75
Compare
craigmarker
force-pushed
the
craig/revision-manager
branch
from
ae5579c to
f2fbc20
Compare
5 tasks
craigmarker
added a commit
that referenced
this pull request
May 15, 2026
…fo (#1144) **What type of PR is this? (check all applicable)** - [x] Refactor - [x] Feature **Why?** Revisions need to carry a `source` tag so consumers can tell how a revision was created — whether by a git push, a resource update, or anything a third-party controller defines. The git-specific flat fields (`git_ref`, `git_branch`, `review_tool_url`) are removed in favor of a structured `CommitInfo` message, which keeps git metadata self-contained. **What changed?** Updated `RevisionSpec` in `proto/api/v2/revision.proto`: - **Added** `string source` (required) — tags how the revision was created; well-known values are `"Git"`, `"ResourceUpdate"`, and `"Unknown"`; third-party controllers can use any value - **Added** `CommitInfo git_commit` — carries git commit metadata for git-backed revisions - **Removed** flat git fields: `git_ref`, `git_branch`, `review_tool_url`, and `commit` Regenerated `proto-go/` bindings via `tools/gen-proto-go.sh`. **How did you test it?** Proto compiles cleanly. The Go implementation in PR #1145 exercises these fields with full unit test coverage. **Potential risks** None — the v2 Revision API has no existing callers. **Release notes** `RevisionSpec` now includes `source` (required string) and `git_commit` (`CommitInfo`). The old flat fields `git_ref`, `git_branch`, `review_tool_url`, and `commit` are removed. **Documentation Changes** None required.
craigmarker
force-pushed
the
craig/revision-manager
branch
from
f2fbc20 to
d4d00d7
Compare
🔍 Go Lint & TODO Tracking Results
|
🔍 Go Lint & TODO Tracking Results
|
craigmarker
force-pushed
the
craig/revision-manager
branch
from
5b126bd to
f53bf2a
Compare
🔍 Go Lint & TODO Tracking Results
|
craigmarker
changed the title
craigmarker
force-pushed
the
craig/revision-manager
branch
from
f53bf2a to
98cb9d1
Compare
🔍 Go Lint & TODO Tracking Results
|
1 similar comment
🔍 Go Lint & TODO Tracking Results
|
craigmarker
force-pushed
the
craig/revision-manager
branch
from
ed493af to
98cb9d1
Compare
🔍 Go Lint & TODO Tracking Results
|
craigmarker
force-pushed
the
craig/revision-manager
branch
from
98cb9d1 to
8ce35dd
Compare
🔍 Go Lint & TODO Tracking Results
|
craigmarker
force-pushed
the
craig/revision-manager
branch
from
8ce35dd to
0fffadb
Compare
🔍 Go Lint & TODO Tracking Results
|
Drops the revision.Manager interface/Module in favor of per-controller Revisioner interfaces (this PR ships pipeline.Revisioner). The revision package becomes a small set of CR-shape helpers (NewCR, LabelSelectorFor, IsAlreadyExists) used by default Revisioner implementations. Pipeline controller depends on pipeline.Revisioner. Default impl writes Revision CRs via the standard k8s API handler, treating AlreadyExists as a no-op. NoOp impl is provided for disabling revisioning per controller. Deployment controller's revision cleanup switches to direct DeleteCollection using revision.LabelSelectorFor (no longer goes through Manager). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
craigmarker
force-pushed
the
craig/revision-manager
branch
from
0fffadb to
556bf8a
Compare
🔍 Go Lint & TODO Tracking Results
|
craigmarker
changed the title
zhoward-1
pushed a commit
that referenced
this pull request
May 20, 2026
…fo (#1144) **What type of PR is this? (check all applicable)** - [x] Refactor - [x] Feature **Why?** Revisions need to carry a `source` tag so consumers can tell how a revision was created — whether by a git push, a resource update, or anything a third-party controller defines. The git-specific flat fields (`git_ref`, `git_branch`, `review_tool_url`) are removed in favor of a structured `CommitInfo` message, which keeps git metadata self-contained. **What changed?** Updated `RevisionSpec` in `proto/api/v2/revision.proto`: - **Added** `string source` (required) — tags how the revision was created; well-known values are `"Git"`, `"ResourceUpdate"`, and `"Unknown"`; third-party controllers can use any value - **Added** `CommitInfo git_commit` — carries git commit metadata for git-backed revisions - **Removed** flat git fields: `git_ref`, `git_branch`, `review_tool_url`, and `commit` Regenerated `proto-go/` bindings via `tools/gen-proto-go.sh`. **How did you test it?** Proto compiles cleanly. The Go implementation in PR #1145 exercises these fields with full unit test coverage. **Potential risks** None — the v2 Revision API has no existing callers. **Release notes** `RevisionSpec` now includes `source` (required string) and `git_commit` (`CommitInfo`). The old flat fields `git_ref`, `git_branch`, `review_tool_url`, and `commit` are removed. **Documentation Changes** None required.
…y semantics The OSS revision package only had stateless helpers (NewCR, LabelSelectorFor). Controllers that need full revision lifecycle — upsert with dedup, lookup, bulk delete — had to inline that logic or go through a per-controller Revisioner abstraction that didn't compose. Add a Manager interface matching the internal revision manager's shape so the pipeline controller (and future controllers) can depend on a single revision lifecycle owner. The implementation handles: - Create-or-update with immutability: creates if not found, dedup-skips if already immutable, rejects immutable→mutable transitions, updates mutable revisions with new content/labels. - GetRevision / FetchRevisionID for lookups. - DeleteAllRevisions using label selectors for cleanup on base resource deletion. Pipeline's defaultRevisioner now delegates to Manager.UpsertRevision instead of calling NewCR + handler.Create directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
manager.go → helpers.go (stateless CR builders and error checks) revision_manager.go → manager.go (Manager implementation) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
Remove unused methods (GetRevision, FetchRevisionID, DeleteAllRevisions) and helpers (LabelSelectorFor, IsAlreadyExists) from the revision package. The Manager interface now exposes only UpsertRevision — the single method with a production caller (pipeline revisioner). Deployment controller: remove revision import and replace inline revision cleanup with a TODO(#534) reference. Deployment revision management is tracked separately in issue #534. Also rename NewCR → NewRevision for clarity and fix a caller-map-mutation bug where labels from params.Labels were modified in place. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
Remove label-writing from NewRevision and the Labels field from UpsertRevisionParams. The Revision proto already has field indexes on spec.base_resource and spec.base_type.kind, so queries should use FieldSelector (matching the internal repo) rather than label selectors. Also remove unused SourceResourceUpdate and SourceUnknown constants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
Manager.UpsertRevision now takes a client.Object instead of UpsertRevisionParams — the caller builds the complete Revision in whatever API version it uses, and the Manager orchestrates the get-or-create state machine without inspecting version-specific fields. This aligns with the internal revision.Manager interface for mergeback. - Remove Revisioner abstraction; pipeline controller depends on revision.Manager directly (matching internal's Params pattern) - Expand Manager interface to match internal: UpsertRevision, CheckRevision, GetRevision, FetchRevisionID, DeleteAllRevisions - Add ...yarpc.CallOption to all methods for internal parity - Delete helpers.go (NewRevision, UpsertRevisionParams) — Revision construction moves to the caller (pipeline's snapshotRevision) - Trim verbose controller.go comments per Effective Go Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
craigmarker
changed the title
The previous 5-method interface (UpsertRevision, CheckRevision, GetRevision, FetchRevisionID, DeleteAllRevisions) wrapped handler calls that callers can invoke directly. Only UpsertRevision has real logic — the immutability state machine. Remove the four thin-wrapper methods so the interface is a single method. Use rev.DeepCopyObject() for the existing-revision lookup instead of hardcoding v2pb.Revision, making both interface.go and manager.go fully version-agnostic with zero proto imports. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
The previous commits trimmed doc comments on functions that weren't otherwise changed by this branch. Restore them so the diff only contains changes related to the revision Manager integration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
… doc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
Snapshot-on-READY was prototyped and tested end-to-end locally but pulled back: deployers who don't want revision storage need an opt-out flag before this can be enabled by default. That wiring lands in a follow-up PR alongside the RevisioningEnabled config knob. - Remove revisionManager field and snapshotRevision from controller - Remove formatRevisionName (no longer used by the reconcile loop) - Remove fx.Provide(revision.NewManager) from Module - Drop TestFormatRevisionName and revision assertions from test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
Previously UpsertRevision returned false on both update and dedup, making it impossible for callers to distinguish a write from a no-op. The only case that should return false is dedup: an existing immutable Revision with the same name, where nothing was written to the store. Updated the interface doc and existing tests to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔍 Go Lint & TODO Tracking Results
|
craigmarker
changed the title
badcount
reviewed
Jun 5, 2026
| // where git-ref-prefix is the first 12 characters (or less) of the git reference. | ||
| // | ||
| // For example: "pipeline-my-model-a1b2c3d4e5f6" | ||
| func formatRevisionName(pipeline *v2pb.Pipeline) string { |
Contributor
Author
There was a problem hiding this comment.
I'm happy to add it back. Technically, revision isn't hooked up to pipeline controller at all today. So there is no "latest revision" of a pipeline. Just the pipeline CR itself.
I'll add this functionality back when we support configuring revisioning per CRD #1268
Contributor
Author
There was a problem hiding this comment.
what do you think?
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
2 participants
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 type of PR is this? (check all applicable)
What changed?
The
revision.Managerinterface and its no-op backing implementation have been replaced with a version-agnostic interface and a real implementation. The new interface includes a single function for revision upserting.It follows the controller-runtime caller-owns-type pattern: callers build a fully-populated Revision in whatever API version they use and pass it as a
client.Object.The deployment controller's no-op
revision.Managerfield is removed. It existed as a placeholder but was never called.Why?
The previous interface was scoped to deployments and backed by a no-op that silently did nothing. Making it version-agnostic unblocks any controller from producing revisions without the Manager needing to know about each resource type. The single-method design keeps the interface minimal: the state machine in
UpsertRevisionis the only behavior that isn't a thin wrapper over the API handler.How did you test it?
Validated the end-to-end revision flow against a sandbox cluster with wiring connected to pipeline controller (removed so it can land behind a config flag): applied a Pipeline CR with a commit field, and confirmed a Revision CR was created with the expected spec fields.
Potential risks
None in this PR — the Manager is defined and tested but not wired into any reconcile loop.