From 082fb43aca1e4be8b8941dc64bc6723b739bd030 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 16 Sep 2026 16:55:50 +0000 Subject: [PATCH 1/3] feat(commit)!: name the resourceVersion a commit wrote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A live commit message could not name any version of the resources it wrote, while a reconcile commit could name the snapshot's. The gap was not in the template layer: sanitize strips metadata.resourceVersion from the object on purpose, so the fact was erased one call before the message was built, although the watch seam holds it and already reads it to join the author fact. Carry it on the Event, beside the object rather than inside it, and expose it as Resources[i].ResourceVersion on liveTemplate and requestTemplate. A DELETE carries one — the watch Deleted frame delivers the final object — which is where it parts company with Kind and Labels. Producers that observed no object (reconcile, bootstrap) leave it empty, so templates guard it with {{with}}. No default template changes. The value is the version of the state the commit wrote, not the object's current one: an update whose committed content would be identical is never routed, so it lags deliberately. It is for joining a commit to an audit log or a kubectl read, and for an equality check. Not for subtraction — resourceVersion is opaque by contract and is the cluster-wide store revision in practice, so a gap measures other objects' writes. watch_events_total{outcome="unchanged"} is the census of what the operator actually skipped. Rename reconcileTemplate's {{.Revision}} to {{.ResourceVersion}} in the same release, so the vocabulary changes once. Revision meant three things here: a Git commit (LayoutReport.Revision, resolvedAtRevision), a watch-plan counter, and — only in this one user-facing place — a Kubernetes resourceVersion. Now Revision is a Git commit and ResourceVersion is a Kubernetes version. The internal chain carrying the snapshot version follows the name, so it is traceable end to end from list.GetResourceVersion() to the template. A stored template still naming {{.Revision}} is rejected with a sentence naming both spellings, via a tombstone method rather than a scan of the template source, so every indirect spelling is caught too. It never returns the value: honouring the old name silently is the upgrade failure this rejects. BREAKING CHANGE: a GitTarget whose spec.commit.message.reconcileTemplate names {{.Revision}} is refused — it reports Validated=False with reason InvalidConfig and stops gating its rules until the template names {{.ResourceVersion}} instead. Targets on the default reconcileTemplate are unaffected: the default is renamed with the field and its output is byte-identical. See docs/UPGRADING.md. Co-Authored-By: Claude Opus 5 --- api/v1alpha3/gitprovider_types.go | 10 +- .../crd/bases/configbutler.ai_gittargets.yaml | 10 +- docs/UPGRADING.md | 49 ++++++ docs/configuration.md | 66 +++++++- .../gittarget_commit_validation_test.go | 13 ++ internal/git/branch_worker_split_test.go | 2 +- internal/git/commit.go | 42 +++-- internal/git/commit_resource_version_test.go | 157 ++++++++++++++++++ internal/git/commit_test.go | 4 +- internal/git/open_window.go | 8 +- .../git/reconcile_revision_rename_test.go | 81 +++++++++ internal/git/resync_flush.go | 8 +- internal/git/resync_push_test.go | 34 ++-- internal/git/types.go | 79 ++++++++- internal/watch/event_router.go | 5 +- internal/watch/target_watch.go | 45 +++-- .../target_watch_resource_version_test.go | 71 ++++++++ test/e2e/signing_e2e_test.go | 11 +- 18 files changed, 613 insertions(+), 82 deletions(-) create mode 100644 internal/git/commit_resource_version_test.go create mode 100644 internal/git/reconcile_revision_rename_test.go create mode 100644 internal/watch/target_watch_resource_version_test.go diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index b8f77fe4..1126a6dc 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -179,7 +179,9 @@ type CommitMessageSpec struct { // LiveTemplate formats every live window, including one resource and a 0s window. // Fields: Author (raw username, possibly empty), GitTarget, Count, Operations, Resources. // Count is retained input entries after coalescing, before comparison with Git. - // Resources exposes Operation, Group, Version, Resource, Namespace, Name, and APIVersion. + // Resources exposes Operation, Group, Version, Resource, Namespace, Name, APIVersion, and + // ResourceVersion — the version of the state this commit wrote, which lags the object's + // current one and is empty for a producer that observed none. Guard it with {{with}}. // A literal CommitRequest message overrides this template without changing authorship. // +optional LiveTemplate string `json:"liveTemplate,omitempty"` @@ -207,9 +209,11 @@ type CommitMessageSpec struct { RequestTemplate string `json:"requestTemplate,omitempty"` // ReconcileTemplate formats atomic snapshots and resyncs. - // Fields: Count, GitTarget, Group, Version, Resource, APIVersion, Namespace, Revision. - // Type and Namespace fields are empty for whole-target snapshots. Revision can be empty. + // Fields: Count, GitTarget, Group, Version, Resource, APIVersion, Namespace, ResourceVersion. + // ResourceVersion is the snapshot LIST's version, not any one object's. Type and Namespace + // fields are empty for whole-target snapshots, and ResourceVersion can be empty. // Guard optional fields so the message remains meaningful for every snapshot scope. + // Revision was renamed to ResourceVersion in v0.48.0; a template still naming it is rejected. // +optional ReconcileTemplate string `json:"reconcileTemplate,omitempty"` diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index b34584ac..dbf32916 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -162,15 +162,19 @@ spec: LiveTemplate formats every live window, including one resource and a 0s window. Fields: Author (raw username, possibly empty), GitTarget, Count, Operations, Resources. Count is retained input entries after coalescing, before comparison with Git. - Resources exposes Operation, Group, Version, Resource, Namespace, Name, and APIVersion. + Resources exposes Operation, Group, Version, Resource, Namespace, Name, APIVersion, and + ResourceVersion — the version of the state this commit wrote, which lags the object's + current one and is empty for a producer that observed none. Guard it with {{with}}. A literal CommitRequest message overrides this template without changing authorship. type: string reconcileTemplate: description: |- ReconcileTemplate formats atomic snapshots and resyncs. - Fields: Count, GitTarget, Group, Version, Resource, APIVersion, Namespace, Revision. - Type and Namespace fields are empty for whole-target snapshots. Revision can be empty. + Fields: Count, GitTarget, Group, Version, Resource, APIVersion, Namespace, ResourceVersion. + ResourceVersion is the snapshot LIST's version, not any one object's. Type and Namespace + fields are empty for whole-target snapshots, and ResourceVersion can be empty. Guard optional fields so the message remains meaningful for every snapshot scope. + Revision was renamed to ResourceVersion in v0.48.0; a template still naming it is rejected. type: string requestTemplate: description: |- diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 0afc16f3..ee32b802 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,55 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## `reconcileTemplate`'s `Revision` is now `ResourceVersion` + +**Breaking, and only for a custom `reconcileTemplate`.** The reconcile commit-message field +`{{.Revision}}` is renamed to `{{.ResourceVersion}}`. A template still naming the old spelling is +**rejected**, not quietly pruned: the `GitTarget` reports `Validated=False` with reason +`InvalidConfig` and a message naming both spellings, and it stops gating its rules until the +template is fixed. + +Nothing changes if you never set `reconcileTemplate`. The default template is renamed with the +field and its output is byte-identical — still +`chore: reconcile 4 configmaps in team-a (last resourceVersion: 1331)`. + +### Why + +`Revision` meant three different things in this project: a Git commit (`LayoutReport.Revision`, the +`GitTarget`'s `resolvedAtRevision`), an internal watch-plan counter, and — only in this one +user-facing place — a Kubernetes `resourceVersion`. A commit message about Git is the last place +"revision" should mean something other than a commit. + +The rename lands in the same release as the new per-resource +[`Resources[i].ResourceVersion`](configuration.md#naming-the-version-a-commit-wrote) on +`liveTemplate`, so the vocabulary changes once: **`Revision` is a Git commit, `ResourceVersion` is a +Kubernetes version.** + +### What to change + +Rename the field in any `reconcileTemplate` that uses it: + +```yaml +spec: + commit: + message: + # before + reconcileTemplate: "chore: reconcile {{.Count}}{{if .Revision}} at {{.Revision}}{{end}}" + # after + reconcileTemplate: "chore: reconcile {{.Count}}{{with .ResourceVersion}} at {{.}}{{end}}" +``` + +Find them before upgrading: + +```bash +kubectl get gittargets -A \ + -o jsonpath='{range .items[?(@.spec.commit.message.reconcileTemplate)]}{.metadata.namespace}{"/"}{.metadata.name}{"\t"}{.spec.commit.message.reconcileTemplate}{"\n"}{end}' \ + | grep -F '.Revision' +``` + +The meaning is unchanged: it is the snapshot `LIST`'s `resourceVersion`, one value for the whole +reconcile, and empty for a pure sweep. + ## `CommitRequest.spec.closeDelaySeconds` defaults to 2 seconds **Breaking.** A `CommitRequest` that omits `closeDelaySeconds` now sets its finalize deadline two diff --git a/docs/configuration.md b/docs/configuration.md index f398ab18..7a755808 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -598,15 +598,15 @@ spec: {{range .Resources -}} - [{{.Operation}}] {{.APIVersion}}/{{.Resource}}/{{if .Namespace}}{{.Namespace}}/{{end}}{{.Name}} {{end -}} - reconcileTemplate: "chore: reconcile {{.Count}} {{if .Resource}}{{.Resource}}{{else}}resources{{end}}{{if .Namespace}} in {{.Namespace}}{{end}}{{if .Revision}} (last resourceVersion: {{.Revision}}){{end}}" + reconcileTemplate: "chore: reconcile {{.Count}} {{if .Resource}}{{.Resource}}{{else}}resources{{end}}{{if .Namespace}} in {{.Namespace}}{{end}}{{if .ResourceVersion}} (last resourceVersion: {{.ResourceVersion}}){{end}}" ``` | Template | Fields | |---|---| | `liveTemplate` | `Author`, `GitTarget`, `Count`, `Operations`, `Resources`, and the `LabelValues` / `LabelValue` accessors | | `requestTemplate` | the same fields, plus `RequestMessage` | -| Each `Resources` entry | `Operation`, `Group`, `Version`, `Resource`, `Kind`, `Namespace`, `Name`, `APIVersion`, `Labels`, and the `Label` accessor | -| `reconcileTemplate` | `Count`, `GitTarget`, `Group`, `Version`, `Resource`, `APIVersion`, `Namespace`, `Revision` | +| Each `Resources` entry | `Operation`, `Group`, `Version`, `Resource`, `Kind`, `Namespace`, `Name`, `APIVersion`, `ResourceVersion`, `Labels`, and the `Label` accessor | +| `reconcileTemplate` | `Count`, `GitTarget`, `Group`, `Version`, `Resource`, `APIVersion`, `Namespace`, `ResourceVersion` | Live `Count` counts retained entries after window coalescing, before the writer compares them with Git. Repeated edits to one resource collapse to one entry, with the last operation retained. @@ -699,12 +699,51 @@ Three things to know: nothing, so a commit holding one labeled and one unlabeled resource is named after neither. - **A `DELETE` carries no object**, because the resource is already gone from the cluster, so `Kind` and `Labels` are empty for one. The identity fields (`Name`, `Namespace`, `Resource`, …) - are unaffected. A commit containing a `DELETE` therefore has no agreed `LabelValue`: what the - deleted resource was labeled is not something the window still knows. + are unaffected, and so is `ResourceVersion`: the watch delivers the final object, so the last + version that existed is still knowable even though the object is not. A commit containing a + `DELETE` therefore has no agreed `LabelValue`: what the deleted resource was labeled is not + something the window still knows. + +##### Naming the version a commit wrote + +`{{.ResourceVersion}}` on a `Resources` entry is the `metadata.resourceVersion` of the state that +commit wrote. It is not committed to the file (`resourceVersion` is stripped from every manifest +on purpose, because a version inside a manifest would make every observation a byte change); it +travels beside the object, in the message only. Guard it with `{{with}}`: + +```yaml +liveTemplate: |- + chore: sync {{.Count}} resource{{if ne .Count 1}}s{{end}} + + {{range .Resources -}} + - [{{.Operation}}] {{.APIVersion}}/{{.Resource}}/{{.Namespace}}/{{.Name}}{{with .ResourceVersion}}@{{.}}{{end}} + {{end -}} +``` + +It is off by default. Turn it on when you want a commit to be **joinable**: to an audit log entry, +to a `kubectl get -o yaml` taken at the time, to another operator's logs. + +Two limits, both worth knowing before you rely on it: + +- **It lags the cluster, deliberately.** An update whose committed content would be identical (a + `/status`-only write) is never routed, so the version in the commit can be older than the + object's current one. A resource re-edited inside one window contributes the last routed + version, and an entry that already matched Git still contributes one. +- **Compare it for equality, never by subtraction.** `resourceVersion` is opaque by API contract, + and is the cluster-wide store revision in practice, so two consecutive commits of one ConfigMap + can read `@1331` then `@8402` with nothing skipped: every other object's writes moved the + same counter. Equal to the live version means nothing is pending; a gap means nothing at all. To + count what the operator skipped, read `watch_events_total{outcome="unchanged"}`, which is exactly + that census. + +A producer that observed no object leaves it empty: a reconcile, a resync, and bootstrap writes all +render nothing there. `reconcileTemplate` gets none of these. It describes a *type* being reconciled rather than a list -of resources, so there are no labels to read, and its `Namespace` keeps the plain meaning it always -had: the namespace a namespace-scoped reconcile covered, empty for a whole-target or all-namespaces +of resources, so there are no labels to read, and its own `{{.ResourceVersion}}` is the snapshot +`LIST`'s version: one value for the whole run, not any single object's. Live names n resources, so +it names each one's version; reconcile names a type, so it names the snapshot's. Its `Namespace` +keeps the plain meaning it always had: the namespace a namespace-scoped reconcile covered, empty for a whole-target or all-namespaces one. That emptiness means "every namespace", not "cluster-scoped", so the `_cluster` sentinel would be a lie there rather than a convenience. @@ -724,6 +763,11 @@ The nouns are the same in both, and only the spelling follows each host language | name | `{name}` | `.Name` | | API version | `{apiVersion}` | `.APIVersion` | | one label | `{label:team}` | `.Label "team"` | +| the observed version | (none) | `.ResourceVersion` | + +The version is the first noun that legitimately exists on only one side. A path keyed on a version +would write a new file on every observation, which is the opposite of what placement is for: a path +has to be stable and statically checkable. A message describes one moment, so it can name one. The capitals are Go's, not a style choice: `text/template` can only reach exported struct fields, which must begin with one. The braces are lower-case because a placement template reads like the @@ -731,8 +775,12 @@ manifest it is filing (`metadata.namespace`). A label needs a method call rather the commit side because a Go template field name cannot contain a `:` or a `/`. Reconcile type fields name the synced type; `Namespace` names a namespace-scoped snapshot. -Whole-target snapshots leave those fields empty. `Revision` is the snapshot's resourceVersion and -can be absent, including a pure sweep. Guard optional values as in the example. +Whole-target snapshots leave those fields empty. `ResourceVersion` is the snapshot's resourceVersion +and can be absent, including a pure sweep. Guard optional values as in the example. + +`ResourceVersion` was called `Revision` before `v0.48.0`. A `reconcileTemplate` still naming +`{{.Revision}}` is rejected, and the `GitTarget` says so on its `Validated` condition. See +[the upgrade note](UPGRADING.md#reconciletemplates-revision-is-now-resourceversion). A reconcile runs per *cell* (a (type, namespace) pair) rather than per target, so a namespace-scoped run covers exactly one namespace and the default subject names it: diff --git a/internal/controller/gittarget_commit_validation_test.go b/internal/controller/gittarget_commit_validation_test.go index 31dfc623..ba242756 100644 --- a/internal/controller/gittarget_commit_validation_test.go +++ b/internal/controller/gittarget_commit_validation_test.go @@ -78,6 +78,19 @@ func TestValidateCommitConfig(t *testing.T) { }, saysA: "spec.commit.message", }, + { + // The renamed reconcile field reaches an operator HERE, on the Validated condition, + // so the message has to name the new spelling. Reaching it through the controller + // (not just ValidateCommitConfig) is the point: this is the text someone upgrading + // into a stored template actually reads. + name: "a reconcile template still naming the retired Revision", + spec: &configbutleraiv1alpha3.GitTargetCommitSpec{ + Message: &configbutleraiv1alpha3.CommitMessageSpec{ + ReconcileTemplate: "chore: reconcile {{.Count}} at {{.Revision}}", + }, + }, + saysA: "{{.ResourceVersion}}", + }, } { t.Run(tc.name, func(t *testing.T) { target := &configbutleraiv1alpha3.GitTarget{} diff --git a/internal/git/branch_worker_split_test.go b/internal/git/branch_worker_split_test.go index cd26f63d..b00b6d2c 100644 --- a/internal/git/branch_worker_split_test.go +++ b/internal/git/branch_worker_split_test.go @@ -1140,7 +1140,7 @@ func TestResync_WorkerAppliesMarkAndSweepAndCommits(t *testing.T) { // Resync: the cluster now has "keep" and no longer has "drop-me". req := &ResyncRequest{ Desired: []manifestanalyzer.DesiredResource{desiredCM("keep", "blue")}, - Revision: "42", + ResourceVersion: "42", GitTargetName: "target-a", GitTargetNamespace: "default", Result: make(chan ResyncResult, 1), diff --git a/internal/git/commit.go b/internal/git/commit.go index 3d12c3de..91a6c24d 100644 --- a/internal/git/commit.go +++ b/internal/git/commit.go @@ -24,8 +24,8 @@ import ( // renderReconcileCommitMessageFromEvents renders the reconcile commit message for the // events-based atomic path from the provider's ReconcileTemplate. It carries no single -// type or revision, so those template fields stay empty (the default guards them). Literal -// overrides are resolved by the caller. +// type and no snapshot version, so those template fields stay empty (the default guards them). +// Literal overrides are resolved by the caller. func renderReconcileCommitMessageFromEvents( events []Event, gitTarget string, @@ -44,21 +44,21 @@ func renderReconcileCommitMessageFromEvents( // renderReconcileCommitMessage renders the reconcile commit message for a resync from the // provider's ReconcileTemplate, so a resync honours a custom reconcile template. count is the // number of resources the reconcile changed; scopeGVR names the synced type for a per-type -// reconcile, and a nil scopeGVR (whole-target reconcile) leaves the type fields empty; revision -// is the cluster resourceVersion the desired set was pinned to (empty for a pure sweep). The -// default template guards the type and revision fields so it still renders cleanly when either -// is absent. +// reconcile, and a nil scopeGVR (whole-target reconcile) leaves the type fields empty; +// resourceVersion is the cluster resourceVersion the desired set was pinned to (empty for a +// pure sweep). The default template guards the type and version fields so it still renders +// cleanly when either is absent. func renderReconcileCommitMessage( count int, gitTarget string, scope *ResyncScope, - revision string, + resourceVersion string, config CommitConfig, ) (string, error) { data := ReconcileCommitMessageData{ - Count: count, - GitTarget: gitTarget, - Revision: revision, + Count: count, + GitTarget: gitTarget, + ResourceVersion: resourceVersion, } if scope != nil { data.Group = scope.Cell.Group @@ -133,7 +133,8 @@ func ValidateCommitConfig(config CommitConfig) error { // Validate the per-type reconcile path with the type and revision fields populated, // so a custom reconcile template that names its synced type ({{.Resource}} / {{.APIVersion}}) - // or pins the {{.Revision}} is exercised at admission exactly as a per-type reconcile renders it. + // or pins the {{.ResourceVersion}} is exercised at admission exactly as a per-type reconcile + // renders it. // The sample scope names a namespace so a template referencing {{.Namespace}} — populated // only by a namespace-scoped reconcile — is validated here too. sampleScope := ResyncScopeFor( @@ -173,6 +174,10 @@ func ValidateCommitConfig(config CommitConfig) error { // It is shared so liveTemplate and requestTemplate cannot drift into being checked against // different worlds. They face identical windows at runtime; checking one more thoroughly than the // other just moves which template fails months later instead of at admission. +// +// The UPDATE sample deliberately carries NO resourceVersion. A live watch stamps one on every +// event, but reconcile and bootstrap do not, and a window is not guaranteed to be one producer's +// — so the shape a template must survive is the mixed one. func liveValidationSamples(sampleEvent Event) [][]Event { var samples [][]Event for _, author := range []string{"template-validator", ""} { @@ -182,6 +187,7 @@ func liveValidationSamples(sampleEvent Event) [][]Event { event.UserInfo.Username = author event.Operation = operation event.Identifier.Name = operation + event.ResourceVersion = sampleResourceVersion(operation) if operation == "CREATE" { event.Object = sampleLabeledObject() } @@ -192,6 +198,20 @@ func liveValidationSamples(sampleEvent Event) [][]Event { return samples } +// sampleResourceVersion gives the validation samples both states of ResourceVersion: present on a +// CREATE and on a DELETE — which carries one although it carries no object — and absent on the +// UPDATE, standing in for every producer that observed no version. +func sampleResourceVersion(operation string) string { + switch operation { + case "CREATE": + return "20001" + case "DELETE": + return "20003" + default: + return "" + } +} + // newRequestTemplateProbe mints the sentinel that requestTemplate validation renders as the // request's message. // diff --git a/internal/git/commit_resource_version_test.go b/internal/git/commit_resource_version_test.go new file mode 100644 index 00000000..12a14462 --- /dev/null +++ b/internal/git/commit_resource_version_test.go @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "strings" + "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/sanitize" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// observedEvent is a live event as the watch seam builds one: the object sanitized, the observed +// resourceVersion carried BESIDE it. +func observedEvent(name, operation, resourceVersion string) Event { + event := Event{ + Identifier: types.NewResourceIdentifier("apps", "v1", "deployments", "prod", name), + Operation: operation, + ResourceVersion: resourceVersion, + } + if operation != "DELETE" { + event.Object = &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{"name": name, "namespace": "prod"}, + }} + } + return event +} + +func TestBuildLiveCommitMessageData_CarriesResourceVersionPerResource(t *testing.T) { + data := buildLiveCommitMessageData("someone", "target", "", []Event{ + observedEvent("api", "CREATE", "20001"), + // A DELETE carries no object, and so no Kind and no labels — but it DOES carry a + // version: the watch Deleted frame delivers the final object, so the last version that + // existed is knowable even though the object is not. + observedEvent("web", "DELETE", "20003"), + // A reconcile-sourced event observed nothing, so it names no version. + {Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "prod", "cm"), Operation: "RECONCILE"}, + }) + + want := []string{"20001", "20003", ""} + for i, w := range want { + if got := data.Resources[i].ResourceVersion; got != w { + t.Errorf("Resources[%d].ResourceVersion = %q, want %q", i, got, w) + } + } + if kind := data.Resources[1].Kind; kind != "" { + t.Errorf("a DELETE must still carry no Kind, got %q", kind) + } +} + +// A resource re-edited inside one window collapses to its last routed event, so the version the +// message names is the state the commit actually writes — not the one that opened the window. +func TestBuildLiveCommitMessageData_ResourceVersionIsTheLastObserved(t *testing.T) { + data := buildLiveCommitMessageData("someone", "target", "", []Event{ + observedEvent("api", "CREATE", "20001"), + observedEvent("api", "UPDATE", "20009"), + }) + + last := data.Resources[len(data.Resources)-1] + if last.ResourceVersion != "20009" { + t.Errorf("last ref names %q, want the newest observed version 20009", last.ResourceVersion) + } +} + +func TestRenderLiveCommitMessage_RendersResourceVersion(t *testing.T) { + config := CommitConfig{Message: CommitMessageConfig{ + LiveTemplate: `chore: sync {{.Count}}` + "\n" + + `{{range .Resources}}- {{.Name}}{{with .ResourceVersion}}@{{.}}{{end}}` + "\n" + `{{end}}`, + }} + write := PendingWrite{Kind: PendingWriteCommit, Events: []Event{ + observedEvent("api", "CREATE", "20001"), + {Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "prod", "cm"), Operation: "RECONCILE"}, + }} + + got, err := renderLiveCommitMessage(write, config) + if err != nil { + t.Fatalf("renderLiveCommitMessage: %v", err) + } + if !strings.Contains(got, "- api@20001") { + t.Errorf("message %q must name the observed version", got) + } + // The {{with}} guard is what keeps a mixed window renderable, so the unversioned entry has + // to come out clean rather than as a trailing "@". + if !strings.Contains(got, "- cm\n") { + t.Errorf("message %q must render an unversioned resource without a dangling separator", got) + } +} + +// The invariant this whole feature rests on: the version reaches the MESSAGE and never the +// CONTENT. A version inside a committed manifest would make every observation a byte change, +// which is exactly what sanitize strips it to prevent — so anyone tempted to "fix" the empty +// field by leaving resourceVersion on the object fails here. +func TestObservedResourceVersionNeverReachesCommittedContent(t *testing.T) { + live := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{ + "name": "api", "namespace": "prod", "resourceVersion": "20001", + }, + }} + + event := Event{ + Identifier: types.NewResourceIdentifier("apps", "v1", "deployments", "prod", "api"), + Operation: "CREATE", + Object: sanitize.Sanitize(live), + ResourceVersion: live.GetResourceVersion(), + } + + if event.ResourceVersion != "20001" { + t.Fatalf("the event must carry the observed version, got %q", event.ResourceVersion) + } + if got := event.Object.GetResourceVersion(); got != "" { + t.Errorf("the committed object still carries resourceVersion %q: it must never reach content", got) + } + if _, found, _ := unstructured.NestedString(event.Object.Object, "metadata", "resourceVersion"); found { + t.Error("metadata.resourceVersion is present in the object the writer commits") + } + // And the source object is untouched, so stamping the event cannot have consumed it. + if live.GetResourceVersion() != "20001" { + t.Error("sanitizing must not strip the version from the observed object") + } +} + +func TestValidateCommitConfig_AcceptsResourceVersionTemplates(t *testing.T) { + config := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ + LiveTemplate: `chore: sync {{.Count}}{{range .Resources}} {{.Name}}` + + `{{with .ResourceVersion}}@{{.}}{{end}}{{end}}`, + ReconcileTemplate: `chore: reconcile {{.Count}}{{with .ResourceVersion}} at {{.}}{{end}}`, + }) + + if err := ValidateCommitConfig(config); err != nil { + t.Errorf("a template naming ResourceVersion must validate: %v", err) + } +} + +// The validation samples must contain BOTH states, or a template that only renders cleanly for a +// versioned resource passes admission and fails months later in a mixed window. +func TestLiveValidationSamples_CoverVersionedAndUnversioned(t *testing.T) { + var versioned, unversioned bool + for _, events := range liveValidationSamples(Event{Operation: "CREATE"}) { + for _, event := range events { + if event.ResourceVersion == "" { + unversioned = true + } else { + versioned = true + } + } + } + if !versioned || !unversioned { + t.Errorf("samples cover versioned=%v unversioned=%v, want both", versioned, unversioned) + } +} diff --git a/internal/git/commit_test.go b/internal/git/commit_test.go index 63f80d18..0ac0adba 100644 --- a/internal/git/commit_test.go +++ b/internal/git/commit_test.go @@ -202,7 +202,7 @@ func TestRenderReconcileCommitMessage_CustomTemplateUsesTypeAndRevisionFields(t "1331", ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ ReconcileTemplate: "e2e-snapshot: synced {{.Count}} {{.APIVersion}}/{{.Resource}}" + - "@{{.Revision}} to {{.GitTarget}}", + "@{{.ResourceVersion}} to {{.GitTarget}}", }), ) require.NoError(t, err) @@ -211,7 +211,7 @@ func TestRenderReconcileCommitMessage_CustomTemplateUsesTypeAndRevisionFields(t func TestValidateCommitConfig_CustomReconcileTemplateReferencingTypeAndRevision(t *testing.T) { config := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ - ReconcileTemplate: "reconcile: {{.Count}} {{.APIVersion}}/{{.Resource}}@{{.Revision}} on {{.GitTarget}}", + ReconcileTemplate: "reconcile: {{.Count}} {{.APIVersion}}/{{.Resource}}@{{.ResourceVersion}} on {{.GitTarget}}", }) require.NoError(t, ValidateCommitConfig(config)) diff --git a/internal/git/open_window.go b/internal/git/open_window.go index b32813c8..a5d2570d 100644 --- a/internal/git/open_window.go +++ b/internal/git/open_window.go @@ -102,7 +102,9 @@ func windowPathKey(e Event, writer eventContentWriter) string { // buildLiveCommitMessageData produces the template context for a grouped // commit unit. Operations are counted by Operation tag; Resources is the -// deduplicated list of resource refs in arrival order. +// deduplicated list of resource refs in arrival order — so a resource re-edited inside the +// window contributes the LAST routed event's resourceVersion, which is the state the commit +// actually writes. // // requestMessage is the attached CommitRequest's message, or empty when no request attached. It is // carried through unaltered — never parsed, never trimmed — because the request's literal bytes @@ -133,6 +135,10 @@ func buildLiveCommitMessageData( Namespace: e.Identifier.NamespaceOrCluster(), Name: e.Identifier.Name, Labels: labels, + // Read off the EVENT, not off Object: sanitize strips resourceVersion from the + // object precisely so it never reaches a committed manifest. That is also why a + // DELETE carries one here while Kind and Labels above stay empty for it. + ResourceVersion: e.ResourceVersion, }) } return LiveCommitMessageData{ diff --git a/internal/git/reconcile_revision_rename_test.go b/internal/git/reconcile_revision_rename_test.go new file mode 100644 index 00000000..a86cd0c7 --- /dev/null +++ b/internal/git/reconcile_revision_rename_test.go @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "strings" + "testing" + + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// A stored reconcileTemplate still naming {{.Revision}} must be refused with a SENTENCE. Without +// the tombstone method the render fails too, but with "can't evaluate field Revision in type +// git.ReconcileCommitMessageData" — which lands on a GitTarget's Validated condition and tells its +// owner nothing about what to write instead. The message is the whole point of the tombstone, so +// the assertion is on the message. +func TestValidateCommitConfig_RejectsRetiredRevisionWithBothSpellings(t *testing.T) { + config := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ + ReconcileTemplate: "chore: reconcile {{.Count}} at {{.Revision}}", + }) + + err := ValidateCommitConfig(config) + if err == nil { + t.Fatal("a reconcileTemplate naming the retired {{.Revision}} must be rejected") + } + for _, want := range []string{"{{.Revision}}", "{{.ResourceVersion}}"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q must name %s so the fix is readable from the condition", err, want) + } + } +} + +// A scan for the literal "{{.Revision}}" would miss every indirect spelling. The tombstone is a +// method, so the render fails wherever the field is reached from. +func TestValidateCommitConfig_RejectsRetiredRevisionIndirectly(t *testing.T) { + for name, template := range map[string]string{ + "with": "chore: reconcile {{.Count}}{{with .Revision}} at {{.}}{{end}}", + "pipeline": `chore: reconcile {{.Count}} at {{.Revision | printf "%s"}}`, + "variable": "{{$rv := .Revision}}chore: reconcile {{.Count}} at {{$rv}}", + "if": "chore: reconcile {{.Count}}{{if .Revision}} pinned{{end}}", + } { + t.Run(name, func(t *testing.T) { + config := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ + ReconcileTemplate: template, + }) + if err := ValidateCommitConfig(config); err == nil { + t.Errorf("%q reaches the retired field and must be rejected", template) + } + }) + } +} + +// The tombstone must not become a compatibility shim: honouring the old name would keep the wrong +// word alive indefinitely, which is the "accepted, and quietly carried on" upgrade failure the +// rejection exists to avoid. +func TestReconcileCommitMessageDataRevision_NeverReturnsTheValue(t *testing.T) { + value, err := ReconcileCommitMessageData{ResourceVersion: "1331"}.Revision() + if err == nil { + t.Fatal("the Revision tombstone must always error") + } + if value != "" { + t.Errorf("the tombstone returned %q: it must never carry the value under the old name", value) + } +} + +// The rename is a rename: the default subject's output is unchanged, so a target on the default +// template crosses the release without noticing. +func TestDefaultReconcileTemplate_SubjectUnchangedByTheRename(t *testing.T) { + scope := ResyncScopeFor( + schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, "team-a") + message, err := renderReconcileCommitMessage(4, "dest", &scope, "1331", ResolveCommitConfig(nil)) + if err != nil { + t.Fatalf("renderReconcileCommitMessage: %v", err) + } + const want = "chore: reconcile 4 deployments in team-a (last resourceVersion: 1331)" + if message != want { + t.Errorf("subject = %q, want %q", message, want) + } +} diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index 28002c45..3fe96269 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -86,7 +86,7 @@ func resyncHealKey(req *ResyncRequest) healKey { func (l *branchWorkerEventLoop) applyResync(req *ResyncRequest) { l.w.Log.Info("Handling resync request", "resources", len(req.Desired), - "revision", req.Revision, + "resourceVersion", req.ResourceVersion, "scope", req.Scope.String(), "heal", req.Heal, "gitTarget", req.GitTargetNamespace+"/"+req.GitTargetName, @@ -193,7 +193,7 @@ func (w *BranchWorker) buildResyncPendingWrite( return &PendingWrite{ Kind: PendingWriteResync, Desired: req.Desired, - Revision: req.Revision, + ResourceVersion: req.ResourceVersion, Scope: req.Scope, ResyncStats: stats, CommitConfig: ResolveCommitConfig(provider.Spec.Commit).WithTargetMessage(targetMetadata.CommitMessage), @@ -296,7 +296,7 @@ func (w *BranchWorker) executeResyncPendingWrite( // commitMetadata through the verbatim path. changed := stats.Created + stats.Updated + stats.Deleted rendered, err := renderReconcileCommitMessage( - changed, target.Name, pendingWrite.Scope, pendingWrite.Revision, pendingWrite.CommitConfig) + changed, target.Name, pendingWrite.Scope, pendingWrite.ResourceVersion, pendingWrite.CommitConfig) if err != nil { return 0, err } @@ -314,7 +314,7 @@ func (w *BranchWorker) executeResyncPendingWrite( log.FromContext(ctx).Info("git resync commit created", "created", stats.Created, "updated", stats.Updated, "deleted", stats.Deleted, "skipped", stats.Skipped, - "placementSkipped", stats.PlacementSkipped, "revision", pendingWrite.Revision) + "placementSkipped", stats.PlacementSkipped, "resourceVersion", pendingWrite.ResourceVersion) return 1, nil } diff --git a/internal/git/resync_push_test.go b/internal/git/resync_push_test.go index 60559ae8..7b69c1ea 100644 --- a/internal/git/resync_push_test.go +++ b/internal/git/resync_push_test.go @@ -62,7 +62,7 @@ func TestEnqueueResync_CoalescesSameScope(t *testing.T) { superseded := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "1", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "1", RefreshRemote: true, Result: superseded, })) @@ -71,7 +71,7 @@ func TestEnqueueResync_CoalescesSameScope(t *testing.T) { // accepted: it replaces the queued one rather than being dropped. newest := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "2", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "2", Result: newest, }), "a resync for an already-queued scope coalesces instead of being dropped") @@ -88,12 +88,12 @@ func TestEnqueueResync_CoalescesSameScope(t *testing.T) { item := <-w.eventQueue require.NotNil(t, item.Resync) current := w.takePendingResync(item.Resync) - assert.Equal(t, "2", current.Revision, "the marker runs the newest request for its scope") + assert.Equal(t, "2", current.ResourceVersion, "the marker runs the newest request for its scope") assert.True(t, current.RefreshRemote, "coalescing must preserve a pending remote refresh") // The key is cleared, so the next resync for that scope queues a fresh marker. require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "3", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "3", Result: make(chan ResyncResult, 1), })) assert.Len(t, w.eventQueue, 1, "a scope taken off the queue can be queued again") @@ -108,18 +108,18 @@ func TestEnqueueResync_CoalescingCarriesTheSurvivingRequest(t *testing.T) { scope := ResyncScopeFor(configmapsGVRForScope, "team-a") require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "1", Scope: &scope, + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "1", Scope: &scope, SourceCell: cell, Result: make(chan ResyncResult, 1), })) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "2", Scope: &scope, + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "2", Scope: &scope, SourceCell: cell, Result: make(chan ResyncResult, 1), })) item := <-w.eventQueue require.NotNil(t, item.Resync) current := w.takePendingResync(item.Resync) - assert.Equal(t, "2", current.Revision, + assert.Equal(t, "2", current.ResourceVersion, "the newer snapshot is the one that runs, at the marker's position") assert.Equal(t, cell, current.SourceCell, "and it still names the cell that gathered it") } @@ -285,7 +285,7 @@ func TestEnqueueResync_DoesNotCoalescePastQueuedWrites(t *testing.T) { first := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "100", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "100", Scope: scope, Result: first, })) @@ -293,7 +293,7 @@ func TestEnqueueResync_DoesNotCoalescePastQueuedWrites(t *testing.T) { require.True(t, w.Enqueue(liveEvent("target", "app"))) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "103", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "103", Scope: scope, Result: make(chan ResyncResult, 1), })) @@ -309,12 +309,12 @@ func TestEnqueueResync_DoesNotCoalescePastQueuedWrites(t *testing.T) { require.Len(t, w.eventQueue, 3, "the later resync takes its own slot rather than coalescing") firstMarker := <-w.eventQueue require.NotNil(t, firstMarker.Resync) - assert.Equal(t, "100", w.takePendingResync(firstMarker.Resync).Revision, + assert.Equal(t, "100", w.takePendingResync(firstMarker.Resync).ResourceVersion, "the earlier marker runs the snapshot it carried, not the newer one") assert.NotNil(t, (<-w.eventQueue).Request, "the write keeps its position between the snapshots") lastMarker := <-w.eventQueue require.NotNil(t, lastMarker.Resync) - assert.Equal(t, "103", w.takePendingResync(lastMarker.Resync).Revision) + assert.Equal(t, "103", w.takePendingResync(lastMarker.Resync).ResourceVersion) } // TestEnqueueResync_DoesNotCoalescePastQueuedAttach pins the same fence for a @@ -325,14 +325,14 @@ func TestEnqueueResync_DoesNotCoalescePastQueuedAttach(t *testing.T) { first := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "100", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "100", Result: first, })) w.EnqueueAttach(&AttachCommitRequest{ Namespace: "ns", Name: "cr", GitTargetNamespace: "ns", GitTargetName: "target", }) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "103", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "103", Result: make(chan ResyncResult, 1), })) @@ -356,7 +356,7 @@ func TestEnqueueResync_CoalescesPastUnrelatedWrites(t *testing.T) { superseded := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "100", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "100", Scope: scope, Result: superseded, })) @@ -365,7 +365,7 @@ func TestEnqueueResync_CoalescesPastUnrelatedWrites(t *testing.T) { require.True(t, w.Enqueue(liveEvent("elsewhere", "app"))) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "103", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "103", Scope: scope, Result: make(chan ResyncResult, 1), })) @@ -378,7 +378,7 @@ func TestEnqueueResync_CoalescesPastUnrelatedWrites(t *testing.T) { require.Len(t, w.eventQueue, 3, "coalescing still costs no extra FIFO slot") marker := <-w.eventQueue require.NotNil(t, marker.Resync) - assert.Equal(t, "103", w.takePendingResync(marker.Resync).Revision) + assert.Equal(t, "103", w.takePendingResync(marker.Resync).ResourceVersion) } // TestEnqueueResync_FenceHoldsUnderConcurrentWrites drives writes and resyncs for one @@ -464,7 +464,7 @@ func raceWritesAndResyncsOnOneScope(t *testing.T) (fifoTally, int, int) { defer wg.Done() if w.EnqueueResync(&ResyncRequest{ GitTargetNamespace: "ns", GitTargetName: "target", - Revision: strconv.Itoa(i), Scope: scope, Result: replies[i], + ResourceVersion: strconv.Itoa(i), Scope: scope, Result: replies[i], }) { resyncs.Add(1) } diff --git a/internal/git/types.go b/internal/git/types.go index 789c4cfb..58c6b655 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -3,6 +3,7 @@ package git import ( + "errors" "fmt" "sort" "strings" @@ -105,7 +106,7 @@ const ( DefaultReconcileCommitMessageTemplate = "chore: reconcile {{.Count}} " + "{{if .Resource}}{{.Resource}}{{else}}resources{{end}}" + "{{if .Namespace}} in {{.Namespace}}{{end}}" + - "{{if .Revision}} (last resourceVersion: {{.Revision}}){{end}}" + "{{if .ResourceVersion}} (last resourceVersion: {{.ResourceVersion}}){{end}}" // DefaultLiveCommitMessageTemplate describes retained input resources. DefaultLiveCommitMessageTemplate = "chore: sync {{.Count}} resource{{if ne .Count 1}}s{{end}}\n\n" + "{{range .Resources -}}" + @@ -289,9 +290,12 @@ type PendingWrite struct { // sweep), and no sibling type's — nor, for a namespace-scoped resync, any sibling // namespace's — document is ever dropped. Nil is the whole-GitTarget resync. Scope *ResyncScope - // Revision is the cluster snapshot resourceVersion the desired set is pinned to - // (the joined streaming-watch bookmark). Carried for diagnostics and logging. - Revision string + // ResourceVersion is the cluster snapshot's resourceVersion the desired set is pinned to + // (the joined streaming-watch bookmark) — the COLLECTION's version, not any one object's. + // Carried for diagnostics, logging, and the reconcile commit message. + // + // Not "Revision": in this codebase a revision is a Git commit (see LayoutReport.Revision). + ResourceVersion string // ResyncStats, when non-nil, is populated during apply with the plan's // create/update/delete/skip counts so a synchronous caller can report them. ResyncStats *ResyncStats @@ -385,14 +389,14 @@ func (s *ResyncScope) Matches(ri types.ResourceIdentifier) bool { } // ResyncRequest is a synchronous resync of one GitTarget against a complete, -// revision-pinned desired snapshot (M8). It rides the worker queue so the single +// version-pinned desired snapshot (M8). It rides the worker queue so the single // git-mutating goroutine applies it in order with live events, and replies on // Result once the local commit is created. The desired set is the whole watched -// resource state at Revision; the worker's content-derived mark-and-sweep drops +// resource state at ResourceVersion; the worker's content-derived mark-and-sweep drops // any managed document the snapshot did not contain. type ResyncRequest struct { Desired []manifestanalyzer.DesiredResource - Revision string + ResourceVersion string GitTargetName string GitTargetNamespace string // Scope, when set, makes this a per-type (M12) reconcile/sweep: the mark-and-sweep is @@ -513,6 +517,20 @@ type Event struct { // Identifier contains resource identification information. Identifier types.ResourceIdentifier + // ResourceVersion is the metadata.resourceVersion of the observed state this event + // describes. It is PROVENANCE, never content: sanitize strips resourceVersion from Object + // on purpose (a version in a committed manifest makes every observation a byte change), so + // this field carries the fact BESIDE the object rather than inside it. Never write it back + // onto Object. + // + // Stamped where the object is observed — the live watch holds the unsanitized object there + // and nowhere downstream does. Producers with no observed object (reconcile, bootstrap) + // leave it empty. + // + // Unlike the object-derived fields, a DELETE CAN carry one: the watch Deleted frame + // delivers the final object, so this names the last version that existed. + ResourceVersion string + // Operation is the admission operation (CREATE, UPDATE, DELETE). Operation string @@ -602,7 +620,7 @@ type CommitMessageConfig struct { // ReconcileCommitMessageData is the template context for reconcile commit messages. // // Group, Version, Resource and APIVersion describe the reconciled type, and are -// populated only for a per-type reconcile. Revision is the resourceVersion the desired set was +// populated only for a per-type reconcile. ResourceVersion is the version the desired set was // pinned to. Any template referencing these must render cleanly when absent; the default guards // both with {{if}}. type ReconcileCommitMessageData struct { @@ -612,12 +630,41 @@ type ReconcileCommitMessageData struct { Version string Resource string APIVersion string - Revision string + // ResourceVersion is THE COLLECTION'S metadata.resourceVersion — the LIST the desired set + // was folded from, not any one object's. A reconcile describes a type, so the version it + // can name is the snapshot's. (The per-object counterpart is + // LiveCommitMessageData.Resources[i].ResourceVersion, which a live commit carries because + // it names n resources.) + // + // Empty for a pure sweep. It was called Revision until v0.48.0; see the Revision tombstone + // below for why the word moved. + ResourceVersion string // Namespace is the single source namespace a namespace-scoped reconcile covered, and // is empty for a whole-target or all-namespaces reconcile. Namespace string } +// Revision is the retired name of ResourceVersion, kept ONLY so a stored template that still +// says {{.Revision}} is refused with a sentence instead of a text/template internal +// ("can't evaluate field Revision in type git.ReconcileCommitMessageData"). +// +// It is a method rather than a scan of the template source because a method catches every +// spelling — {{with .Revision}}, {{.Revision | printf "%s"}}, assignment to a variable — which a +// search for the literal string would not. Same argument validateRequestTemplate makes for +// probing behaviour rather than matching template text. +// +// It deliberately does NOT return the value. Silently honouring the old name would keep the +// wrong word alive indefinitely, and "accepted, and quietly carried on" is the upgrade failure +// mode docs/UPGRADING.md is written against. Reject, do not prune. +// +// Delete one minor release after v0.48.0. By then {{.Revision}} can go back to failing as a +// plain unknown field, which is all an unknown field deserves. +func (ReconcileCommitMessageData) Revision() (string, error) { + return "", errors.New( + "reconcileTemplate: {{.Revision}} was renamed to {{.ResourceVersion}} in v0.48.0 " + + "(it is the snapshot's resourceVersion; \"revision\" now only ever means a Git commit)") +} + // ResourceRef is the lightweight resource identifier emitted to grouped commit // templates via LiveCommitMessageData.Resources. // @@ -650,6 +697,20 @@ type ResourceRef struct { // // Read it with .Label, not with .Labels.key — see Label. Labels map[string]string + // ResourceVersion is the metadata.resourceVersion of the observed state this commit wrote, + // and "" for a producer that observed none (reconcile, bootstrap). Guard it with + // {{with .ResourceVersion}} — empty here has one meaning, "this producer observed no + // version", and no sentinel states that better than silence. + // + // It is NOT the object's current version, and the gap is deliberate: an update whose + // git-writable content is unchanged (a /status-only write) is never routed, so this value + // lags the cluster's. Compare it for EQUALITY — equal means nothing is pending — never by + // subtraction: resourceVersion is opaque by contract and is the global store revision in + // practice, so a gap measures other objects' writes, not our own drops. The census + // counts those: watch_events_total{outcome="unchanged"}. + // + // Unlike Kind and Labels, a DELETE DOES carry one — see Event.ResourceVersion. + ResourceVersion string } // Label is the value of one label on this resource, and "" when it does not carry the label. diff --git a/internal/watch/event_router.go b/internal/watch/event_router.go index 390c81f1..c32f5626 100644 --- a/internal/watch/event_router.go +++ b/internal/watch/event_router.go @@ -194,13 +194,14 @@ func (r *EventRouter) resolveWorkerForGitDest( // worker defers while a commit window is open. enqueued is false when the worker's queue was full // and dropped the request (its failure is still delivered on resultCh for the drain to record). // refreshRemote asks the worker to fetch the latest remote tip before it inspects the folder. +// resourceVersion is the LIST's collection version the desired set is pinned to. func (r *EventRouter) enqueueScopedResync( ctx context.Context, gitDest types.ResourceReference, scope git.ResyncScope, sourceCell types.CellKey, desired []manifestanalyzer.DesiredResource, - revision string, + resourceVersion string, heal bool, refreshRemote bool, ) (chan git.ResyncResult, bool, error) { @@ -211,7 +212,7 @@ func (r *EventRouter) enqueueScopedResync( resultCh := make(chan git.ResyncResult, 1) enqueued := worker.EnqueueResync(&git.ResyncRequest{ Desired: desired, - Revision: revision, + ResourceVersion: resourceVersion, GitTargetName: gitDest.Name, GitTargetNamespace: gitDest.Namespace, Scope: &scope, diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index ed8efb25..b4ee938e 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -810,11 +810,11 @@ func (m *Manager) targetWatchListAndStream( return fmt.Errorf("list target watch snapshot %s/%q: %w", stream.key.GVR.String(), stream.key.Namespace, err) } desired := desiredFromList(stream.key.GVR, list) - revision := list.GetResourceVersion() - if err := m.enqueueReplayResync(ctx, log, gitDest, stream, desired, revision); err != nil { + resourceVersion := list.GetResourceVersion() + if err := m.enqueueReplayResync(ctx, log, gitDest, stream, desired, resourceVersion); err != nil { return err } - if err := m.recordTargetWatchCursor(ctx, gitDest, stream.key, revision); err != nil { + if err := m.recordTargetWatchCursor(ctx, gitDest, stream.key, resourceVersion); err != nil { return err } // Recorded HERE, not where the fallback was chosen. watch_recovery_total counts recoveries that @@ -824,7 +824,7 @@ func (m *Manager) targetWatchListAndStream( m.recordWatchRecovery(gitDest, stream.key.GVR.Group, stream.key.GVR.Resource, recoveryModeListFallback) log.Info("target watch list fallback complete", "gitDest", gitDest.String(), "gvr", stream.key.GVR.String(), "namespace", stream.key.Namespace, - "count", len(desired), "resourceVersion", revision) + "count", len(desired), "resourceVersion", resourceVersion) m.markTargetStreamState( gitDest, stream.key.Cell(), @@ -832,7 +832,7 @@ func (m *Manager) targetWatchListAndStream( StreamReasonAllStreamsReady, "target watch list fallback complete", ) - return m.streamLiveTargetWatchEvents(ctx, log, gitDest, stream, buffered, revision) + return m.streamLiveTargetWatchEvents(ctx, log, gitDest, stream, buffered, resourceVersion) } func (m *Manager) handleTargetWatchSessionEvent( @@ -916,7 +916,7 @@ func (m *Manager) enqueueReplayResync( gitDest types.ResourceReference, stream targetWatchStream, desired []manifestanalyzer.DesiredResource, - revision string, + resourceVersion string, ) error { if m.EventRouter == nil { return nil @@ -930,13 +930,14 @@ func (m *Manager) enqueueReplayResync( return nil default: } - // The revision is the one this stream was STARTED with, never the cell's current revision. - // A cancelled stream can still be in flight with a replay result, and reading the revision - // here would let it report a scope clean under a revision it never replayed for — reopening - // writes on the strength of a snapshot the new plan never gathered. The gate already - // ignores a superseded revision; capturing it at start is what makes it stale. + // The stream's PLAN revision (stream.revision, a render-fidelity generation counter — not the + // resourceVersion this snapshot is pinned to) is the one this stream was STARTED with, never + // the cell's current one. A cancelled stream can still be in flight with a replay result, and + // reading that revision here would let it report a scope clean under a revision it never + // replayed for — reopening writes on the strength of a snapshot the new plan never gathered. + // The gate already ignores a superseded revision; capturing it at start is what makes it stale. resultCh, enqueued, err := m.EventRouter.enqueueScopedResync( - ctx, gitDest, resyncScopeForWatchKey(stream.key), stream.sourceCell(), desired, revision, false, + ctx, gitDest, resyncScopeForWatchKey(stream.key), stream.sourceCell(), desired, resourceVersion, false, stream.refreshRemote) if err != nil { return err @@ -958,7 +959,8 @@ func (m *Manager) enqueueReplayResync( stream.key.GVR.String(), gitDest.String(), git.ErrFinalizeQueueFull) } log.V(1).Info("target replay resync enqueued", - "gitDest", gitDest.String(), "gvr", stream.key.GVR.String(), "revision", revision, "count", len(desired)) + "gitDest", gitDest.String(), "gvr", stream.key.GVR.String(), + "resourceVersion", resourceVersion, "count", len(desired)) return nil } @@ -1251,10 +1253,23 @@ func sanitizedContentHash(event *git.Event) (string, bool) { return string(sum[:]), true } +// targetWatchGitEvent adapts one observed object into the Event the pipeline carries. +// +// The resourceVersion is stamped here BECAUSE this is where the unsanitized object is: sanitize +// strips it from the object below (a version inside a committed manifest makes every +// observation a byte change), and nothing downstream can recover it — by the time the event +// reaches a branch worker the object is out of the process and the cluster has moved on. It is +// a capture-at-the-seam fact, like the author. +// +// It is stamped for a DELETE too, where Object deliberately is not: the watch Deleted frame +// delivers the final object, so the last version that existed is knowable even though the +// object no longer is. func targetWatchGitEvent(gvr schema.GroupVersionResource, u *unstructured.Unstructured, op string) git.Event { event := git.Event{ - Identifier: types.NewResourceIdentifier(gvr.Group, gvr.Version, gvr.Resource, u.GetNamespace(), u.GetName()), - Operation: op, + Identifier: types.NewResourceIdentifier( + gvr.Group, gvr.Version, gvr.Resource, u.GetNamespace(), u.GetName()), + Operation: op, + ResourceVersion: u.GetResourceVersion(), } if op != string(configv1alpha3.OperationDelete) { event.Object = sanitize.Sanitize(u) diff --git a/internal/watch/target_watch_resource_version_test.go b/internal/watch/target_watch_resource_version_test.go new file mode 100644 index 00000000..392f8570 --- /dev/null +++ b/internal/watch/target_watch_resource_version_test.go @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func observedDeployment(resourceVersion string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{ + "name": "api", "namespace": "prod", "resourceVersion": resourceVersion, + "uid": "2f1c8f1e-0000-4000-8000-000000000001", + }, + "spec": map[string]any{"replicas": int64(2)}, + }} +} + +func deploymentsGVR() schema.GroupVersionResource { + return schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} +} + +// The watch seam is the only place the unsanitized object exists — by the time the event reaches a +// branch worker the object is out of the process and the cluster has moved on. So the version is +// stamped here or it is lost, exactly like the author. +func TestTargetWatchGitEvent_StampsObservedResourceVersion(t *testing.T) { + event := targetWatchGitEvent(deploymentsGVR(), observedDeployment("20001"), "UPDATE") + + if event.ResourceVersion != "20001" { + t.Errorf("event.ResourceVersion = %q, want the observed 20001", event.ResourceVersion) + } + if event.Object == nil { + t.Fatal("an UPDATE must carry the sanitized object") + } + if got := event.Object.GetResourceVersion(); got != "" { + t.Errorf("the object the writer commits still carries resourceVersion %q", got) + } +} + +// A DELETE carries no object — the resource is gone — but the Deleted frame still delivers the +// final one, so the last version that existed IS knowable. This is where the version parts company +// with Kind and Labels, which stay empty for a delete. +func TestTargetWatchGitEvent_DeleteCarriesTheFinalResourceVersion(t *testing.T) { + event := targetWatchGitEvent(deploymentsGVR(), observedDeployment("20003"), "DELETE") + + if event.Object != nil { + t.Error("a DELETE must carry no object") + } + if event.ResourceVersion != "20003" { + t.Errorf("event.ResourceVersion = %q, want the final 20003", event.ResourceVersion) + } +} + +// An object the API server gave no version (a synthesized or hand-built one) leaves the field +// empty rather than inventing a value; templates guard it with {{with}}. +func TestTargetWatchGitEvent_NoVersionObservedStaysEmpty(t *testing.T) { + bare := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{"name": "api", "namespace": "prod"}, + }} + + if got := targetWatchGitEvent(deploymentsGVR(), bare, "CREATE").ResourceVersion; got != "" { + t.Errorf("ResourceVersion = %q, want empty when nothing was observed", got) + } +} diff --git a/test/e2e/signing_e2e_test.go b/test/e2e/signing_e2e_test.go index 4a811523..8346d240 100644 --- a/test/e2e/signing_e2e_test.go +++ b/test/e2e/signing_e2e_test.go @@ -394,10 +394,11 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { watchRuleName := providerName + "-wr" commitPath := "e2e/signing-reconcile" // The reconcile template names the synced type ({{.APIVersion}}/{{.Resource}}) and pins the - // {{.Revision}} so the per-type reconcile commits are self-describing — the §9 "name the synced - // type" improvement in docs/finished/signing-snapshot-tail-replay-failure-investigation.md. + // {{.ResourceVersion}} so the per-type reconcile commits are self-describing — the §9 "name the + // synced type" improvement in docs/finished/signing-snapshot-tail-replay-failure-investigation.md + // (which predates the rename and still writes it as {{.Revision}}). customReconcileTemplate := "e2e-reconcile: synced {{.Count}} {{.APIVersion}}/{{.Resource}}" + - "@{{.Revision}} to {{.GitTarget}}" + "@{{.ResourceVersion}} to {{.GitTarget}}" DeferCleanup(func() { if skipCleanupBecauseResourcesArePreserved( @@ -553,7 +554,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { By("creating target A and its WatchRule, then waiting for A to reconcile the seed band") createValidatedGitTargetWithCommitMessage(destNameA, testNs, providerName, commitPathA, gitTargetCommitOptions{ReconcileTemplate: "e2e-reconcile: synced {{.Count}} " + - "{{.APIVersion}}/{{.Resource}}@{{.Revision}} to {{.GitTarget}}"}) + "{{.APIVersion}}/{{.Resource}}@{{.ResourceVersion}} to {{.GitTarget}}"}) Expect(applyFromTemplate("test/e2e/templates/watchrule.tmpl", struct { Name, Namespace, DestinationName string }{watchRuleNameA, testNs, destNameA}, testNs)).To(Succeed()) @@ -580,7 +581,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { Expect(err).NotTo(HaveOccurred(), "failed to create overlap-b configmaps") createGitTargetWithCommitMessage(destNameB, testNs, providerName, commitPathB, "main", gitTargetCommitOptions{ReconcileTemplate: "e2e-reconcile: synced {{.Count}} " + - "{{.APIVersion}}/{{.Resource}}@{{.Revision}} to {{.GitTarget}}"}) + "{{.APIVersion}}/{{.Resource}}@{{.ResourceVersion}} to {{.GitTarget}}"}) Expect(applyFromTemplate("test/e2e/templates/watchrule.tmpl", struct { Name, Namespace, DestinationName string }{watchRuleNameB, testNs, destNameB}, testNs)).To(Succeed()) From 05d40970ac2da60b0b2a9fe171ef3b5d5db32143 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 16 Sep 2026 17:08:53 +0000 Subject: [PATCH 2/3] feat(commit): name the generation a commit wrote, beside the version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resourceVersion moves on every write, status included, and is opaque by contract — so it answers "is anything pending?" and nothing about what a commit missed. generation is the counter that does: it tracks the DESIRED state, is per object, starts at 1, and advances once per spec write, so a gap between two commits really is spec changes that were not committed separately. The repo's own corpus records the distinction: the deployment status-update scenario holds generation at 1 across watch.modified.1 and .2 while resourceVersion moves. Neither replaces the other, so both are carried and the docs say where each is blind. A ConfigMap, a Secret, and every other kind without a spec carry no generation at all — confirmed against the same corpus, where only deployment, pod and widget have one — and a label-only edit changes committed content without moving it. 0 means absent, which {{with .Generation}} already renders as nothing. Same seam as the version: sanitize strips generation one line after resourceVersion, so it is stamped from the unsanitized object at the watch seam and carried beside Object, never inside it. The guard test covers both fields in both directions. Co-Authored-By: Claude Opus 5 --- api/v1alpha3/gitprovider_types.go | 5 +- .../crd/bases/configbutler.ai_gittargets.yaml | 5 +- docs/configuration.md | 76 +++++++++++-------- internal/git/commit.go | 26 ++++++- internal/git/commit_resource_version_test.go | 57 ++++++++++---- internal/git/open_window.go | 8 +- internal/git/types.go | 20 +++++ internal/watch/target_watch.go | 17 +++-- .../target_watch_resource_version_test.go | 55 +++++++++++--- 9 files changed, 194 insertions(+), 75 deletions(-) diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index 1126a6dc..45b40923 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -179,9 +179,10 @@ type CommitMessageSpec struct { // LiveTemplate formats every live window, including one resource and a 0s window. // Fields: Author (raw username, possibly empty), GitTarget, Count, Operations, Resources. // Count is retained input entries after coalescing, before comparison with Git. - // Resources exposes Operation, Group, Version, Resource, Namespace, Name, APIVersion, and + // Resources exposes Operation, Group, Version, Resource, Namespace, Name, APIVersion, // ResourceVersion — the version of the state this commit wrote, which lags the object's - // current one and is empty for a producer that observed none. Guard it with {{with}}. + // current one and is empty for a producer that observed none — and Generation, the desired + // state's counter, which is 0 for a kind without a spec. Guard both with {{with}}. // A literal CommitRequest message overrides this template without changing authorship. // +optional LiveTemplate string `json:"liveTemplate,omitempty"` diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index dbf32916..d0044b10 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -162,9 +162,10 @@ spec: LiveTemplate formats every live window, including one resource and a 0s window. Fields: Author (raw username, possibly empty), GitTarget, Count, Operations, Resources. Count is retained input entries after coalescing, before comparison with Git. - Resources exposes Operation, Group, Version, Resource, Namespace, Name, APIVersion, and + Resources exposes Operation, Group, Version, Resource, Namespace, Name, APIVersion, ResourceVersion — the version of the state this commit wrote, which lags the object's - current one and is empty for a producer that observed none. Guard it with {{with}}. + current one and is empty for a producer that observed none — and Generation, the desired + state's counter, which is 0 for a kind without a spec. Guard both with {{with}}. A literal CommitRequest message overrides this template without changing authorship. type: string reconcileTemplate: diff --git a/docs/configuration.md b/docs/configuration.md index 7a755808..722159b3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -605,7 +605,7 @@ spec: |---|---| | `liveTemplate` | `Author`, `GitTarget`, `Count`, `Operations`, `Resources`, and the `LabelValues` / `LabelValue` accessors | | `requestTemplate` | the same fields, plus `RequestMessage` | -| Each `Resources` entry | `Operation`, `Group`, `Version`, `Resource`, `Kind`, `Namespace`, `Name`, `APIVersion`, `ResourceVersion`, `Labels`, and the `Label` accessor | +| Each `Resources` entry | `Operation`, `Group`, `Version`, `Resource`, `Kind`, `Namespace`, `Name`, `APIVersion`, `ResourceVersion`, `Generation`, `Labels`, and the `Label` accessor | | `reconcileTemplate` | `Count`, `GitTarget`, `Group`, `Version`, `Resource`, `APIVersion`, `Namespace`, `ResourceVersion` | Live `Count` counts retained entries after window coalescing, before the writer compares them with @@ -699,45 +699,59 @@ Three things to know: nothing, so a commit holding one labeled and one unlabeled resource is named after neither. - **A `DELETE` carries no object**, because the resource is already gone from the cluster, so `Kind` and `Labels` are empty for one. The identity fields (`Name`, `Namespace`, `Resource`, …) - are unaffected, and so is `ResourceVersion`: the watch delivers the final object, so the last - version that existed is still knowable even though the object is not. A commit containing a + are unaffected, and so are `ResourceVersion` and `Generation`: the watch delivers the final + object, so the last state that existed is still knowable even though the object is not. A commit containing a `DELETE` therefore has no agreed `LabelValue`: what the deleted resource was labeled is not something the window still knows. -##### Naming the version a commit wrote +##### Naming the state a commit wrote -`{{.ResourceVersion}}` on a `Resources` entry is the `metadata.resourceVersion` of the state that -commit wrote. It is not committed to the file (`resourceVersion` is stripped from every manifest -on purpose, because a version inside a manifest would make every observation a byte change); it -travels beside the object, in the message only. Guard it with `{{with}}`: +Two counters describe the observed state each entry was at. Neither is committed to the file +(`resourceVersion` and `generation` are stripped from every manifest on purpose, because a counter +inside a manifest would make every observation a byte change); both travel beside the object, in +the message only. Both are off by default, and both are guarded with `{{with}}`: ```yaml liveTemplate: |- chore: sync {{.Count}} resource{{if ne .Count 1}}s{{end}} {{range .Resources -}} - - [{{.Operation}}] {{.APIVersion}}/{{.Resource}}/{{.Namespace}}/{{.Name}}{{with .ResourceVersion}}@{{.}}{{end}} + - [{{.Operation}}] {{.APIVersion}}/{{.Resource}}/{{.Namespace}}/{{.Name}}{{with .ResourceVersion}}@{{.}}{{end}}{{with .Generation}} gen{{.}}{{end}} {{end -}} ``` -It is off by default. Turn it on when you want a commit to be **joinable**: to an audit log entry, -to a `kubectl get -o yaml` taken at the time, to another operator's logs. - -Two limits, both worth knowing before you rely on it: - -- **It lags the cluster, deliberately.** An update whose committed content would be identical (a - `/status`-only write) is never routed, so the version in the commit can be older than the - object's current one. A resource re-edited inside one window contributes the last routed - version, and an entry that already matched Git still contributes one. -- **Compare it for equality, never by subtraction.** `resourceVersion` is opaque by API contract, - and is the cluster-wide store revision in practice, so two consecutive commits of one ConfigMap - can read `@1331` then `@8402` with nothing skipped: every other object's writes moved the - same counter. Equal to the live version means nothing is pending; a gap means nothing at all. To - count what the operator skipped, read `watch_events_total{outcome="unchanged"}`, which is exactly - that census. - -A producer that observed no object leaves it empty: a reconcile, a resync, and bootstrap writes all -render nothing there. +| | `{{.ResourceVersion}}` | `{{.Generation}}` | +|---|---|---| +| moves when | **anything** is written, `/status` included | only the **desired state** changes | +| present on | every resource the operator observed | only kinds with a spec: **not** a ConfigMap or a Secret | +| absent value | `""` | `0` | +| safe to compare | for **equality** only | yes: per object, starts at `1`, one step per spec write | + +**Turn `ResourceVersion` on to make a commit joinable**: to an audit log entry, to a +`kubectl get -o yaml` taken at the time, to another operator's logs. Equal to the object's current +version means nothing is pending. Do not subtract two of them: `resourceVersion` is opaque by API +contract and is the cluster-wide store revision in practice, so two consecutive commits of one +ConfigMap can read `@1331` then `@8402` with nothing skipped, because every other object's writes +moved the same counter. + +**Turn `Generation` on to see what a commit missed.** It is the counter a gap is meaningful in: a +commit at `gen3` following one at `gen6` means three spec changes that were not committed +separately. That is the question `ResourceVersion` cannot answer. + +Read them together, because each is blind where the other sees: + +- **`ResourceVersion` lags, deliberately.** An update whose committed content would be identical (a + `/status`-only write) is never routed, so the version in a commit can be older than the object's + current one. A resource re-edited inside one window contributes the last routed version, and an + entry that already matched Git still contributes one. +- **`Generation` misses metadata.** A label- or annotation-only edit changes what gets committed + without moving it, so an unchanged `Generation` across two commits does not mean an unchanged + commit. And it is `0` for every spec-less kind, which is much of what a typical target mirrors. +- **Neither counts skips.** To count what the operator deliberately did not route, read + `watch_events_total{outcome="unchanged"}`, which is exactly that census. + +A producer that observed no object renders nothing for either: reconcile, resync, and bootstrap +writes all leave both empty. `reconcileTemplate` gets none of these. It describes a *type* being reconciled rather than a list of resources, so there are no labels to read, and its own `{{.ResourceVersion}}` is the snapshot @@ -764,10 +778,12 @@ The nouns are the same in both, and only the spelling follows each host language | API version | `{apiVersion}` | `.APIVersion` | | one label | `{label:team}` | `.Label "team"` | | the observed version | (none) | `.ResourceVersion` | +| the desired-state counter | (none) | `.Generation` | -The version is the first noun that legitimately exists on only one side. A path keyed on a version -would write a new file on every observation, which is the opposite of what placement is for: a path -has to be stable and statically checkable. A message describes one moment, so it can name one. +The two counters are the first nouns that legitimately exist on only one side. A path keyed on a +counter would write a new file whenever it moved, which is the opposite of what placement is for: a +path has to be stable and statically checkable. A message describes one moment, so it can name +one. The capitals are Go's, not a style choice: `text/template` can only reach exported struct fields, which must begin with one. The braces are lower-case because a placement template reads like the diff --git a/internal/git/commit.go b/internal/git/commit.go index 91a6c24d..e3aa2c96 100644 --- a/internal/git/commit.go +++ b/internal/git/commit.go @@ -175,9 +175,10 @@ func ValidateCommitConfig(config CommitConfig) error { // different worlds. They face identical windows at runtime; checking one more thoroughly than the // other just moves which template fails months later instead of at admission. // -// The UPDATE sample deliberately carries NO resourceVersion. A live watch stamps one on every -// event, but reconcile and bootstrap do not, and a window is not guaranteed to be one producer's -// — so the shape a template must survive is the mixed one. +// The UPDATE sample deliberately carries NO resourceVersion and NO generation. A live watch +// stamps a version on every event, but reconcile and bootstrap do not, and a generation is absent +// for every kind without a spec (a ConfigMap, a Secret) however it was produced — so the shape a +// template must survive is the mixed one. func liveValidationSamples(sampleEvent Event) [][]Event { var samples [][]Event for _, author := range []string{"template-validator", ""} { @@ -188,6 +189,7 @@ func liveValidationSamples(sampleEvent Event) [][]Event { event.Operation = operation event.Identifier.Name = operation event.ResourceVersion = sampleResourceVersion(operation) + event.Generation = sampleGeneration(operation) if operation == "CREATE" { event.Object = sampleLabeledObject() } @@ -212,6 +214,24 @@ func sampleResourceVersion(operation string) string { } } +// sampleGeneration mirrors sampleResourceVersion for the desired-state counter, so a template +// naming {{.Generation}} is validated against a resource that has one and a resource that does +// not — the second being every spec-less kind this operator mirrors. +func sampleGeneration(operation string) int64 { + const ( + sampleCreateGeneration = 3 + sampleDeleteGeneration = 4 + ) + switch operation { + case "CREATE": + return sampleCreateGeneration + case "DELETE": + return sampleDeleteGeneration + default: + return 0 + } +} + // newRequestTemplateProbe mints the sentinel that requestTemplate validation renders as the // request's message. // diff --git a/internal/git/commit_resource_version_test.go b/internal/git/commit_resource_version_test.go index 12a14462..86d9353f 100644 --- a/internal/git/commit_resource_version_test.go +++ b/internal/git/commit_resource_version_test.go @@ -14,12 +14,13 @@ import ( ) // observedEvent is a live event as the watch seam builds one: the object sanitized, the observed -// resourceVersion carried BESIDE it. -func observedEvent(name, operation, resourceVersion string) Event { +// counters carried BESIDE it. generation 0 stands in for a kind that has none. +func observedEvent(name, operation, resourceVersion string, generation int64) Event { event := Event{ Identifier: types.NewResourceIdentifier("apps", "v1", "deployments", "prod", name), Operation: operation, ResourceVersion: resourceVersion, + Generation: generation, } if operation != "DELETE" { event.Object = &unstructured.Unstructured{Object: map[string]any{ @@ -33,11 +34,11 @@ func observedEvent(name, operation, resourceVersion string) Event { func TestBuildLiveCommitMessageData_CarriesResourceVersionPerResource(t *testing.T) { data := buildLiveCommitMessageData("someone", "target", "", []Event{ - observedEvent("api", "CREATE", "20001"), + observedEvent("api", "CREATE", "20001", 3), // A DELETE carries no object, and so no Kind and no labels — but it DOES carry a // version: the watch Deleted frame delivers the final object, so the last version that // existed is knowable even though the object is not. - observedEvent("web", "DELETE", "20003"), + observedEvent("web", "DELETE", "20003", 4), // A reconcile-sourced event observed nothing, so it names no version. {Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "prod", "cm"), Operation: "RECONCILE"}, }) @@ -48,6 +49,12 @@ func TestBuildLiveCommitMessageData_CarriesResourceVersionPerResource(t *testing t.Errorf("Resources[%d].ResourceVersion = %q, want %q", i, got, w) } } + wantGenerations := []int64{3, 4, 0} + for i, w := range wantGenerations { + if got := data.Resources[i].Generation; got != w { + t.Errorf("Resources[%d].Generation = %d, want %d", i, got, w) + } + } if kind := data.Resources[1].Kind; kind != "" { t.Errorf("a DELETE must still carry no Kind, got %q", kind) } @@ -57,14 +64,17 @@ func TestBuildLiveCommitMessageData_CarriesResourceVersionPerResource(t *testing // message names is the state the commit actually writes — not the one that opened the window. func TestBuildLiveCommitMessageData_ResourceVersionIsTheLastObserved(t *testing.T) { data := buildLiveCommitMessageData("someone", "target", "", []Event{ - observedEvent("api", "CREATE", "20001"), - observedEvent("api", "UPDATE", "20009"), + observedEvent("api", "CREATE", "20001", 3), + observedEvent("api", "UPDATE", "20009", 4), }) last := data.Resources[len(data.Resources)-1] if last.ResourceVersion != "20009" { t.Errorf("last ref names %q, want the newest observed version 20009", last.ResourceVersion) } + if last.Generation != 4 { + t.Errorf("last ref names generation %d, want the newest observed 4", last.Generation) + } } func TestRenderLiveCommitMessage_RendersResourceVersion(t *testing.T) { @@ -73,7 +83,7 @@ func TestRenderLiveCommitMessage_RendersResourceVersion(t *testing.T) { `{{range .Resources}}- {{.Name}}{{with .ResourceVersion}}@{{.}}{{end}}` + "\n" + `{{end}}`, }} write := PendingWrite{Kind: PendingWriteCommit, Events: []Event{ - observedEvent("api", "CREATE", "20001"), + observedEvent("api", "CREATE", "20001", 3), {Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "prod", "cm"), Operation: "RECONCILE"}, }} @@ -100,7 +110,7 @@ func TestObservedResourceVersionNeverReachesCommittedContent(t *testing.T) { "apiVersion": "apps/v1", "kind": "Deployment", "metadata": map[string]any{ - "name": "api", "namespace": "prod", "resourceVersion": "20001", + "name": "api", "namespace": "prod", "resourceVersion": "20001", "generation": int64(3), }, }} @@ -109,27 +119,34 @@ func TestObservedResourceVersionNeverReachesCommittedContent(t *testing.T) { Operation: "CREATE", Object: sanitize.Sanitize(live), ResourceVersion: live.GetResourceVersion(), + Generation: live.GetGeneration(), } - if event.ResourceVersion != "20001" { - t.Fatalf("the event must carry the observed version, got %q", event.ResourceVersion) + if event.ResourceVersion != "20001" || event.Generation != 3 { + t.Fatalf("the event must carry the observed counters, got %q/%d", + event.ResourceVersion, event.Generation) } if got := event.Object.GetResourceVersion(); got != "" { t.Errorf("the committed object still carries resourceVersion %q: it must never reach content", got) } - if _, found, _ := unstructured.NestedString(event.Object.Object, "metadata", "resourceVersion"); found { - t.Error("metadata.resourceVersion is present in the object the writer commits") + if got := event.Object.GetGeneration(); got != 0 { + t.Errorf("the committed object still carries generation %d: it must never reach content", got) + } + for _, field := range []string{"resourceVersion", "generation"} { + if _, found, _ := unstructured.NestedFieldNoCopy(event.Object.Object, "metadata", field); found { + t.Errorf("metadata.%s is present in the object the writer commits", field) + } } // And the source object is untouched, so stamping the event cannot have consumed it. - if live.GetResourceVersion() != "20001" { - t.Error("sanitizing must not strip the version from the observed object") + if live.GetResourceVersion() != "20001" || live.GetGeneration() != 3 { + t.Error("sanitizing must not strip the counters from the observed object") } } func TestValidateCommitConfig_AcceptsResourceVersionTemplates(t *testing.T) { config := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ LiveTemplate: `chore: sync {{.Count}}{{range .Resources}} {{.Name}}` + - `{{with .ResourceVersion}}@{{.}}{{end}}{{end}}`, + `{{with .ResourceVersion}}@{{.}}{{end}}{{with .Generation}} gen{{.}}{{end}}{{end}}`, ReconcileTemplate: `chore: reconcile {{.Count}}{{with .ResourceVersion}} at {{.}}{{end}}`, }) @@ -141,7 +158,7 @@ func TestValidateCommitConfig_AcceptsResourceVersionTemplates(t *testing.T) { // The validation samples must contain BOTH states, or a template that only renders cleanly for a // versioned resource passes admission and fails months later in a mixed window. func TestLiveValidationSamples_CoverVersionedAndUnversioned(t *testing.T) { - var versioned, unversioned bool + var versioned, unversioned, generated, ungenerated bool for _, events := range liveValidationSamples(Event{Operation: "CREATE"}) { for _, event := range events { if event.ResourceVersion == "" { @@ -149,9 +166,17 @@ func TestLiveValidationSamples_CoverVersionedAndUnversioned(t *testing.T) { } else { versioned = true } + if event.Generation == 0 { + ungenerated = true + } else { + generated = true + } } } if !versioned || !unversioned { t.Errorf("samples cover versioned=%v unversioned=%v, want both", versioned, unversioned) } + if !generated || !ungenerated { + t.Errorf("samples cover generation present=%v absent=%v, want both", generated, ungenerated) + } } diff --git a/internal/git/open_window.go b/internal/git/open_window.go index a5d2570d..694ab8f5 100644 --- a/internal/git/open_window.go +++ b/internal/git/open_window.go @@ -135,10 +135,12 @@ func buildLiveCommitMessageData( Namespace: e.Identifier.NamespaceOrCluster(), Name: e.Identifier.Name, Labels: labels, - // Read off the EVENT, not off Object: sanitize strips resourceVersion from the - // object precisely so it never reaches a committed manifest. That is also why a - // DELETE carries one here while Kind and Labels above stay empty for it. + // Read off the EVENT, not off Object: sanitize strips resourceVersion and + // generation from the object precisely so neither reaches a committed manifest. + // That is also why a DELETE carries them here while Kind and Labels above stay + // empty for it. ResourceVersion: e.ResourceVersion, + Generation: e.Generation, }) } return LiveCommitMessageData{ diff --git a/internal/git/types.go b/internal/git/types.go index 58c6b655..14f4892e 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -531,6 +531,14 @@ type Event struct { // delivers the final object, so this names the last version that existed. ResourceVersion string + // Generation is the metadata.generation of the observed state — the DESIRED state's counter, + // where ResourceVersion counts every write. Same provenance-not-content rule: sanitize strips + // generation from Object one line after resourceVersion, and for the same reason. + // + // 0 means "no generation", which covers both "this producer observed nothing" and "this kind + // has none": a ConfigMap or a Secret never carries one, only types with a spec do. + Generation int64 + // Operation is the admission operation (CREATE, UPDATE, DELETE). Operation string @@ -711,6 +719,18 @@ type ResourceRef struct { // // Unlike Kind and Labels, a DELETE DOES carry one — see Event.ResourceVersion. ResourceVersion string + // Generation is the metadata.generation of the state this commit wrote, and 0 when there is + // none. It moves only when the DESIRED state changes, so unlike ResourceVersion it is worth + // comparing: it is per object, starts at 1, and advances once per spec write, so a gap + // between two commits really does mean spec changes that were not committed separately. + // + // Two blind spots keep it from replacing ResourceVersion. A ConfigMap, a Secret, and any + // other kind without a spec never carry one, so this stays 0 for much of what a target + // mirrors. And a label- or annotation-only edit changes what gets committed WITHOUT moving + // it, so an unchanged Generation does not mean an unchanged commit. + // + // Guard it with {{with .Generation}}, which renders nothing for 0. + Generation int64 } // Label is the value of one label on this resource, and "" when it does not carry the label. diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index b4ee938e..695d6600 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -1255,21 +1255,22 @@ func sanitizedContentHash(event *git.Event) (string, bool) { // targetWatchGitEvent adapts one observed object into the Event the pipeline carries. // -// The resourceVersion is stamped here BECAUSE this is where the unsanitized object is: sanitize -// strips it from the object below (a version inside a committed manifest makes every -// observation a byte change), and nothing downstream can recover it — by the time the event -// reaches a branch worker the object is out of the process and the cluster has moved on. It is -// a capture-at-the-seam fact, like the author. +// The resourceVersion and generation are stamped here BECAUSE this is where the unsanitized +// object is: sanitize strips both from the object below (a version inside a committed manifest +// makes every observation a byte change), and nothing downstream can recover them — by the time +// the event reaches a branch worker the object is out of the process and the cluster has moved +// on. They are capture-at-the-seam facts, like the author. // -// It is stamped for a DELETE too, where Object deliberately is not: the watch Deleted frame -// delivers the final object, so the last version that existed is knowable even though the -// object no longer is. +// They are stamped for a DELETE too, where Object deliberately is not: the watch Deleted frame +// delivers the final object, so the last state that existed is knowable even though the object +// no longer is. func targetWatchGitEvent(gvr schema.GroupVersionResource, u *unstructured.Unstructured, op string) git.Event { event := git.Event{ Identifier: types.NewResourceIdentifier( gvr.Group, gvr.Version, gvr.Resource, u.GetNamespace(), u.GetName()), Operation: op, ResourceVersion: u.GetResourceVersion(), + Generation: u.GetGeneration(), } if op != string(configv1alpha3.OperationDelete) { event.Object = sanitize.Sanitize(u) diff --git a/internal/watch/target_watch_resource_version_test.go b/internal/watch/target_watch_resource_version_test.go index 392f8570..cf978b0e 100644 --- a/internal/watch/target_watch_resource_version_test.go +++ b/internal/watch/target_watch_resource_version_test.go @@ -9,13 +9,14 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" ) -func observedDeployment(resourceVersion string) *unstructured.Unstructured { +func observedDeployment(resourceVersion string, generation int64) *unstructured.Unstructured { return &unstructured.Unstructured{Object: map[string]any{ "apiVersion": "apps/v1", "kind": "Deployment", "metadata": map[string]any{ "name": "api", "namespace": "prod", "resourceVersion": resourceVersion, - "uid": "2f1c8f1e-0000-4000-8000-000000000001", + "generation": generation, + "uid": "2f1c8f1e-0000-4000-8000-000000000001", }, "spec": map[string]any{"replicas": int64(2)}, }} @@ -26,27 +27,33 @@ func deploymentsGVR() schema.GroupVersionResource { } // The watch seam is the only place the unsanitized object exists — by the time the event reaches a -// branch worker the object is out of the process and the cluster has moved on. So the version is -// stamped here or it is lost, exactly like the author. +// branch worker the object is out of the process and the cluster has moved on. So the counters are +// stamped here or they are lost, exactly like the author. func TestTargetWatchGitEvent_StampsObservedResourceVersion(t *testing.T) { - event := targetWatchGitEvent(deploymentsGVR(), observedDeployment("20001"), "UPDATE") + event := targetWatchGitEvent(deploymentsGVR(), observedDeployment("20001", 3), "UPDATE") if event.ResourceVersion != "20001" { t.Errorf("event.ResourceVersion = %q, want the observed 20001", event.ResourceVersion) } + if event.Generation != 3 { + t.Errorf("event.Generation = %d, want the observed 3", event.Generation) + } if event.Object == nil { t.Fatal("an UPDATE must carry the sanitized object") } if got := event.Object.GetResourceVersion(); got != "" { t.Errorf("the object the writer commits still carries resourceVersion %q", got) } + if got := event.Object.GetGeneration(); got != 0 { + t.Errorf("the object the writer commits still carries generation %d", got) + } } // A DELETE carries no object — the resource is gone — but the Deleted frame still delivers the // final one, so the last version that existed IS knowable. This is where the version parts company // with Kind and Labels, which stay empty for a delete. func TestTargetWatchGitEvent_DeleteCarriesTheFinalResourceVersion(t *testing.T) { - event := targetWatchGitEvent(deploymentsGVR(), observedDeployment("20003"), "DELETE") + event := targetWatchGitEvent(deploymentsGVR(), observedDeployment("20003", 4), "DELETE") if event.Object != nil { t.Error("a DELETE must carry no object") @@ -54,18 +61,44 @@ func TestTargetWatchGitEvent_DeleteCarriesTheFinalResourceVersion(t *testing.T) if event.ResourceVersion != "20003" { t.Errorf("event.ResourceVersion = %q, want the final 20003", event.ResourceVersion) } + if event.Generation != 4 { + t.Errorf("event.Generation = %d, want the final 4", event.Generation) + } +} + +// A ConfigMap is the case that matters for Generation: the API server gives spec-less kinds none, +// and this operator mirrors them constantly. The field stays 0 rather than inventing a value, and +// templates guard it with {{with}}. The resourceVersion is still there, which is the whole reason +// the two counters are carried separately. +func TestTargetWatchGitEvent_SpeclessKindCarriesNoGeneration(t *testing.T) { + configMap := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{"name": "cm", "namespace": "prod", "resourceVersion": "20005"}, + "data": map[string]any{"key": "value"}, + }} + gvr := schema.GroupVersionResource{Version: "v1", Resource: "configmaps"} + + event := targetWatchGitEvent(gvr, configMap, "CREATE") + if event.Generation != 0 { + t.Errorf("Generation = %d, want 0 for a kind that has none", event.Generation) + } + if event.ResourceVersion != "20005" { + t.Errorf("ResourceVersion = %q, want it present even without a generation", event.ResourceVersion) + } } -// An object the API server gave no version (a synthesized or hand-built one) leaves the field -// empty rather than inventing a value; templates guard it with {{with}}. -func TestTargetWatchGitEvent_NoVersionObservedStaysEmpty(t *testing.T) { +// An object the API server gave neither counter leaves both at their zero value. +func TestTargetWatchGitEvent_NothingObservedStaysEmpty(t *testing.T) { bare := &unstructured.Unstructured{Object: map[string]any{ "apiVersion": "apps/v1", "kind": "Deployment", "metadata": map[string]any{"name": "api", "namespace": "prod"}, }} - if got := targetWatchGitEvent(deploymentsGVR(), bare, "CREATE").ResourceVersion; got != "" { - t.Errorf("ResourceVersion = %q, want empty when nothing was observed", got) + event := targetWatchGitEvent(deploymentsGVR(), bare, "CREATE") + if event.ResourceVersion != "" || event.Generation != 0 { + t.Errorf("counters = %q/%d, want both empty when nothing was observed", + event.ResourceVersion, event.Generation) } } From 5e59eb4ca391674843039c3ef60ad14e1876ee33 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 16 Sep 2026 17:35:41 +0000 Subject: [PATCH 3/3] fix(commit): address review on the commit-template counters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the PR review, each verified against the code first. The UPGRADING link pointed at #naming-the-version-a-commit-wrote, which stopped existing when folding Generation in renamed that heading to "Naming the state a commit wrote". doccheck resolves FILES, not heading anchors, so nothing caught it. The coalescing test did not test coalescing. buildLiveCommitMessageData appends one ResourceRef per event it is handed; deduplication happens in openWindow.add, which is last-write-wins per windowPathKey. Feeding both events straight to the builder and reading the last element asserted nothing but slice indexing, and would have kept passing if coalescing stopped selecting the final event. It now builds the window, asserts the two events collapsed to ONE resource, and only then reads the counters off the survivor. The signing spec recreated its GitTarget without the custom reconcile template, so the recreated target differed from the original in exactly the field the assertion reads. It passes today only because the fresh reconcile finds the batch already committed byte-identically and writes nothing, leaving the pre-recreation commit under assertion — a trap that springs the moment that reconcile has anything to write. Co-Authored-By: Claude Opus 5 --- docs/UPGRADING.md | 2 +- internal/git/commit_resource_version_test.go | 42 ++++++++++++++------ test/e2e/signing_e2e_test.go | 12 +++++- 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index ee32b802..612b5ecf 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -27,7 +27,7 @@ user-facing place — a Kubernetes `resourceVersion`. A commit message about Git "revision" should mean something other than a commit. The rename lands in the same release as the new per-resource -[`Resources[i].ResourceVersion`](configuration.md#naming-the-version-a-commit-wrote) on +[`Resources[i].ResourceVersion`](configuration.md#naming-the-state-a-commit-wrote) on `liveTemplate`, so the vocabulary changes once: **`Revision` is a Git commit, `ResourceVersion` is a Kubernetes version.** diff --git a/internal/git/commit_resource_version_test.go b/internal/git/commit_resource_version_test.go index 86d9353f..53c7187c 100644 --- a/internal/git/commit_resource_version_test.go +++ b/internal/git/commit_resource_version_test.go @@ -62,18 +62,36 @@ func TestBuildLiveCommitMessageData_CarriesResourceVersionPerResource(t *testing // A resource re-edited inside one window collapses to its last routed event, so the version the // message names is the state the commit actually writes — not the one that opened the window. -func TestBuildLiveCommitMessageData_ResourceVersionIsTheLastObserved(t *testing.T) { - data := buildLiveCommitMessageData("someone", "target", "", []Event{ - observedEvent("api", "CREATE", "20001", 3), - observedEvent("api", "UPDATE", "20009", 4), - }) - - last := data.Resources[len(data.Resources)-1] - if last.ResourceVersion != "20009" { - t.Errorf("last ref names %q, want the newest observed version 20009", last.ResourceVersion) - } - if last.Generation != 4 { - t.Errorf("last ref names generation %d, want the newest observed 4", last.Generation) +// +// It goes through openWindow, which is where coalescing actually happens: add() is +// last-write-wins per windowPathKey and orderedEvents() yields one event per path. +// buildLiveCommitMessageData does NOT deduplicate — it appends one ResourceRef per event it is +// handed — so feeding it both events directly and reading the last element would assert nothing +// but slice indexing, and would keep passing if coalescing stopped selecting the final event. +// Hence the length assertion first: one entry is the property, the counters on it are the payload. +func TestOpenWindow_ResourceVersionIsTheLastObserved(t *testing.T) { + first := observedEvent("api", "CREATE", "20001", 3) + second := observedEvent("api", "UPDATE", "20009", 4) + + window := newOpenWindow(first, newContentWriter(types.SensitiveResourcePolicy{})) + window.add(first) + window.add(second) + + data := buildLiveCommitMessageData( + window.Author, window.GitTarget, "", window.orderedEvents()) + + if len(data.Resources) != 1 { + t.Fatalf("the window collapsed to %d resources, want 1 — both events write the same path", + len(data.Resources)) + } + surviving := data.Resources[0] + if surviving.ResourceVersion != "20009" { + t.Errorf("the surviving ref names %q, want the newest observed version 20009", + surviving.ResourceVersion) + } + if surviving.Generation != 4 { + t.Errorf("the surviving ref names generation %d, want the newest observed 4", + surviving.Generation) } } diff --git a/test/e2e/signing_e2e_test.go b/test/e2e/signing_e2e_test.go index 8346d240..294dd510 100644 --- a/test/e2e/signing_e2e_test.go +++ b/test/e2e/signing_e2e_test.go @@ -453,7 +453,17 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { By("recreating the GitTarget now that the WatchRule is active to force a fresh reconcile batch") cleanupGitTarget(destName, testNs) - createValidatedGitTarget(destName, testNs, providerName, commitPath) + // The recreated target must carry the SAME reconcile template. It is the batch commit this + // spec asserts on, and the recreation exists only to re-trigger the reconcile that produces + // it — recreating with the default template instead makes the two targets differ in the one + // way the assertion reads. Today the spec still passes, because the fresh reconcile finds + // the batch already committed byte-identically and writes nothing, so the commit under + // assertion is the pre-recreation one. That is a trap, not a design: the moment the + // recreated reconcile has anything to write, the subject becomes "chore: reconcile ..." and + // the HavePrefix("e2e-reconcile:") assertion below fails for a reason that has nothing to + // do with what this spec tests. + createValidatedGitTargetWithCommitMessage(destName, testNs, providerName, commitPath, + gitTargetCommitOptions{ReconcileTemplate: customReconcileTemplate}) By("waiting for the batch commit and verifying its message uses the batch template") Eventually(func(g Gomega) {