diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index 68cdc9d1..b8f77fe4 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -184,6 +184,28 @@ type CommitMessageSpec struct { // +optional LiveTemplate string `json:"liveTemplate,omitempty"` + // Rationale, kept out of the doc comment (and so out of the CRD schema) by the blank line + // below: a CommitRequest is created by whoever holds create on it in a namespace, so its + // author supplies the message but must not set commit-message policy. Framing the message + // rather than parsing it keeps that split — the operator owns the wording, the requester owns + // the words, and nothing a requester writes is ever executed. Rejecting a template that never + // renders RequestMessage is what keeps the split honest in the other direction: silently + // discarding a requester's stated reason would be worse than having no template at all. + + // RequestTemplate frames a CommitRequest's message instead of committing it verbatim. It + // renders only for a commit window whose attached request supplied a message; every other + // window uses liveTemplate or reconcileTemplate. + // + // It receives the same fields as liveTemplate, plus RequestMessage carrying the request's + // message unaltered. That message is never parsed as a template, so template syntax inside it + // stays literal. + // + // Omitted, a request's message is committed exactly as supplied. A template that never renders + // RequestMessage is rejected. One that fails to render at commit time commits the message + // verbatim instead, counted as message_source="commit_request_fallback". + // +optional + 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. diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index 235f5f77..b34584ac 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -172,6 +172,20 @@ spec: Type and Namespace fields are empty for whole-target snapshots. Revision can be empty. Guard optional fields so the message remains meaningful for every snapshot scope. type: string + requestTemplate: + description: |- + RequestTemplate frames a CommitRequest's message instead of committing it verbatim. It + renders only for a commit window whose attached request supplied a message; every other + window uses liveTemplate or reconcileTemplate. + + It receives the same fields as liveTemplate, plus RequestMessage carrying the request's + message unaltered. That message is never parsed as a template, so template syntax inside it + stays literal. + + Omitted, a request's message is committed exactly as supplied. A template that never renders + RequestMessage is rejected. One that fails to render at commit time commits the message + verbatim instead, counted as message_source="commit_request_fallback". + type: string type: object x-kubernetes-validations: - message: eventTemplate is retired; migrate to liveTemplate diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index cefc9dbb..48bb5cb4 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,35 @@ 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. +## Save messages can be framed by the GitTarget + +**Not breaking.** `GitTarget.spec.commit.message.requestTemplate` is new and optional; omit it and +`CommitRequest.spec.message` is committed verbatim exactly as before. + +Until now a save request's message REPLACED the commit template, so supplying one lost the resource +body `liveTemplate` would have produced. `requestTemplate` composes them: + +```yaml +spec: + commit: + message: + requestTemplate: |- + {{.RequestMessage}} + + {{range .Resources -}} + - [{{.Operation}}] {{.APIVersion}}/{{.Resource}}/{{.Namespace}}/{{.Name}} + {{end -}} +``` + +`CommitRequest.spec.message` stays literal and is still never parsed as a template — it arrives as +`.RequestMessage` and is committed unaltered. A `requestTemplate` that never renders it is rejected +with `Validated=False`. + +`git_commits_total` gains two `message_source` values, `commit_request_framed` and +`commit_request_fallback`. The existing `commit_request` keeps its meaning (a verbatim message with +no `requestTemplate` configured), so dashboards reading it are unaffected. Alert on the fallback +rate: it is a successful commit, so it is the only signal that a template has stopped applying. + ## Watch reconnects no longer report as failures **Not breaking, but two observable surfaces move.** Neither needs a manifest change; both may need diff --git a/docs/configuration.md b/docs/configuration.md index 9d021102..4605a68a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -575,10 +575,13 @@ write rather than stopping the mirror. `reconcileTemplate` formats atomic snapshots and resyncs. Invalid templates report `Validated=False` with reason `InvalidConfig`; validation exercises singleton and mixed-operation windows, empty authors, and scoped and whole-target snapshots through the production renderer. -Sample execution cannot prove every possible conditional branch valid. +Sample execution cannot prove every possible conditional branch valid. That applies to +`requestTemplate`'s [message check](#framing-a-save-message) too: a template that drops the save +message only on some branch passes validation and falls back at commit time. | Input, in precedence order | Message source | |---|---| +| Attached [save request](#commitrequest) **and** `requestTemplate` set | `requestTemplate`, with the request's message as `.RequestMessage` | | Non-empty literal override, including an attached [save request](#commitrequest) | Exact supplied text | | Live window of any size | `liveTemplate` | | Atomic snapshot or resync | `reconcileTemplate` | @@ -601,6 +604,7 @@ spec: | 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` | @@ -611,10 +615,54 @@ first-seen order. An entry already matching Git still counts, and several entrie The count can exceed the number of changed resources. A no-op creates no commit, even with a literal message. Printing a resource entry directly keeps its `group/version/resource[/namespace]/name` form. +`RequestMessage` belongs to `requestTemplate`. It exists on the live context too, but is always +empty there: a window carrying a save request's message renders `requestTemplate` when one is +configured and the literal message when one is not, so `liveTemplate` only ever runs for windows +that have no request message. Do not reach for `{{if .RequestMessage}}` inside `liveTemplate`; +it never fires. + `Author` is the raw window username and is empty when no actor is named. It does not use an OIDC display name or the `attribution-unresolved` Git author sentinel. The sentinel appears only in the Git author header when attribution ran without resolving an actor. Messages never change authorship. +##### Framing a save message + +By default a [save request](#commitrequest)'s message **replaces** the template, so supplying one +costs you the resource body `liveTemplate` would have produced: the commit says why, but no longer +says what. `requestTemplate` composes the two. + +```yaml +spec: + commit: + message: + requestTemplate: |- + {{.RequestMessage}} + + {{range .Resources -}} + - [{{.Operation}}] {{.APIVersion}}/{{.Resource}}/{{.Namespace}}/{{.Name}} + {{end -}} +``` + +It renders only for a window a save request attached to; every other window is unaffected. Omit it +and request messages are committed verbatim, exactly as before. + +The request is **never** parsed as a template. Its message arrives as `.RequestMessage` and is +committed unaltered, so a save-button user supplies the content while the operator owns the +wording around it. Nothing a requester writes is ever executed, and braces inside a request message +stay literal in every case. + +A `requestTemplate` that never renders `.RequestMessage` is **rejected** with `Validated=False`, +because dropping the requester's stated reason is the one thing this field must not do. Every +spelling that puts the message in the commit is accepted (`{{.RequestMessage}}`, a pipeline, or a +variable), because the check renders the template and looks for the message in the output rather +than scanning the template's text. + +`requestTemplate` is validated against the same window shapes as `liveTemplate`, so a template +reading a label some resources do not carry fails at admission rather than at commit time. If one +does fail to render in production, the request's message is committed verbatim instead of the +window being lost, and the commit is counted under `message_source="commit_request_fallback"`. +Alert on that rate: the commit itself succeeds and no condition moves, so it is the only signal. + ##### Kind, scope, and labels `Kind` is the commit-message spelling of the `{kind}` [placement variable](#template-variables), @@ -1614,7 +1662,9 @@ The entire spec is immutable. Create a new `CommitRequest` for each save attempt A present message accepts 1–1024 Unicode characters, including newline. All other ASCII control characters, including tab, carriage return, and DEL, are rejected, as is whitespace-only text. Accepted surrounding spaces are preserved. Braces such as `{{.Author}}` remain literal; omission -uses [the live template](#commit-message-templates). A rejected request leaves automatic mirroring +uses [the live template](#commit-message-templates). To frame a save message with what was saved, +configure [`requestTemplate`](#framing-a-save-message) on the GitTarget. The request is still never +parsed as a template. A rejected request leaves automatic mirroring available. The submitter chooses any semantic prefix; free-form messages are accepted. A request attaches to at most one matching open window. Normal flush triggers may close it early; diff --git a/docs/interpreting-metrics.md b/docs/interpreting-metrics.md index cbc6b737..42b84dbc 100644 --- a/docs/interpreting-metrics.md +++ b/docs/interpreting-metrics.md @@ -353,6 +353,23 @@ serve several GitTargets sharing a provider+branch, coalescing their writes into the worker is the honest attribution unit. `author_kind` is `user`, `serviceaccount`, `committer`, or `unresolved`; reconcile/resync commits and configured-author mode use `committer`. +`message_source` is `live`, `reconcile`, or one of three request values: `commit_request` (a save +request's message committed verbatim, no `requestTemplate` configured), `commit_request_framed` +(`requestTemplate` rendered), and `commit_request_fallback` (`requestTemplate` failed to render, so +the message was committed verbatim instead). + +The fallback is the one to alert on. It is a **successful** commit — nothing is refused and no +condition moves — so without this counter a `requestTemplate` that has quietly stopped applying +looks exactly like one that was never configured: + +```promql +sum by (provider_namespace, provider_name) ( + rate(gitopsreverser_git_commits_total{message_source="commit_request_fallback"}[15m]) +) > 0 +``` + +`framed / (framed + fallback)` is the health ratio for the feature. + **`unresolved` is the one to watch.** It means attribution RAN and did not name an actor, so the commit carries the `unknown (attribution unresolved)` author instead of a person. It is deliberately not folded into `user` (which would make a lost actor look like a named one, so a diff --git a/docs/spec/commitrequest-design.md b/docs/spec/commitrequest-design.md index 2111b7cc..30b8c7d6 100644 --- a/docs/spec/commitrequest-design.md +++ b/docs/spec/commitrequest-design.md @@ -31,8 +31,13 @@ at most one window and cannot rename a finalized commit, including one waiting f provides no ordering guarantee; use a non-zero window for custom save messages. The delay does not reserve a transaction. Competing requests keep the earliest-finalize-deadline selection policy. -`spec.message` is literal, including template-like text and surrounding spaces. Omission uses -`GitTarget.spec.commit.message.liveTemplate`. A present value accepts 1–1024 Unicode characters; +`spec.message` is literal, including template-like text and surrounding spaces. It is never parsed +as a template, so a request author cannot execute one. Omission uses +`GitTarget.spec.commit.message.liveTemplate`. A target may set +`GitTarget.spec.commit.message.requestTemplate` to frame the message with the window's resources; +the message still arrives unaltered, as `.RequestMessage`, and a template that does not render it +is rejected at admission, so the request's bytes always reach the commit. A `requestTemplate` that +fails to render commits the message verbatim rather than losing the window. A present value accepts 1–1024 Unicode characters; newline is allowed, other ASCII controls and whitespace-only text are rejected. Validation never truncates accepted text. A no-op still creates no commit. The message does not change Git identities. diff --git a/internal/git/commit.go b/internal/git/commit.go index b69f88f0..3d12c3de 100644 --- a/internal/git/commit.go +++ b/internal/git/commit.go @@ -4,6 +4,9 @@ package git import ( "bytes" + "crypto/rand" + "encoding/hex" + "errors" "fmt" "regexp" "strings" @@ -68,11 +71,20 @@ func renderReconcileCommitMessage( } func renderLiveCommitMessage(pendingWrite PendingWrite, config CommitConfig) (string, error) { - return renderCommitTemplate( - "live", - config.Message.LiveTemplate, - buildLiveCommitMessageData(pendingWrite.Author(), pendingWrite.Target().Name, pendingWrite.Events), - ) + return renderCommitTemplate("live", config.Message.LiveTemplate, pendingWrite.liveMessageData()) +} + +// renderRequestCommitMessage frames an attached CommitRequest's message with the target's +// requestTemplate. The literal rides in as .RequestMessage rather than being parsed, so nothing a +// requester wrote is ever executed. +func renderRequestCommitMessage(pendingWrite PendingWrite, config CommitConfig) (string, error) { + return renderCommitTemplate("request", config.Message.RequestTemplate, pendingWrite.liveMessageData()) +} + +// liveMessageData is the template context both live renders share. One builder, because a framed +// request commit is a live window that happens to carry a message — not a different kind of commit. +func (p PendingWrite) liveMessageData() LiveCommitMessageData { + return buildLiveCommitMessageData(p.Author(), p.Target().Name, p.CommitMessage, p.Events) } func renderCommitTemplate(name, text string, data any) (string, error) { @@ -139,6 +151,30 @@ func ValidateCommitConfig(config CommitConfig) error { // not carry "team", and failing at admission is the difference between a rejected GitTarget // and a commit that dies mid-window months later. "{{.Label \"team\"}}" renders empty and // passes both. + for _, events := range liveValidationSamples(sampleEvent) { + if _, err := renderLiveCommitMessage(PendingWrite{ + Kind: PendingWriteCommit, Events: events, + }, config); err != nil { + return err + } + if err := validateRequestTemplate(config, events); err != nil { + return err + } + } + + return nil +} + +// liveValidationSamples is the set of window shapes both live-message templates are validated +// against: growing windows, mixed operations, an empty author, and — the important half — a +// resource that carries an object with labels beside ones that carry neither, because these +// templates run with missingkey=error and it is the render WITHOUT the label that fails. +// +// 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. +func liveValidationSamples(sampleEvent Event) [][]Event { + var samples [][]Event for _, author := range []string{"template-validator", ""} { var events []Event for _, operation := range []string{"CREATE", "UPDATE", "DELETE"} { @@ -150,14 +186,69 @@ func ValidateCommitConfig(config CommitConfig) error { event.Object = sampleLabeledObject() } events = append(events, event) - if _, err := renderLiveCommitMessage(PendingWrite{ - Kind: PendingWriteCommit, Events: events, - }, config); err != nil { - return err - } + samples = append(samples, append([]Event(nil), events...)) } } + return samples +} + +// newRequestTemplateProbe mints the sentinel that requestTemplate validation renders as the +// request's message. +// +// Fresh per call, and unpredictable, because the check asks "did the message reach the commit?" by +// looking for this string in the rendered output. A FIXED sentinel answers a weaker question: a +// template that emits the constant itself would pass while never referencing .RequestMessage at +// all. Nobody would write that on purpose, but a check that can be satisfied without doing the +// thing it verifies is not a check. +func newRequestTemplateProbe() (string, error) { + var raw [16]byte + if _, err := rand.Read(raw[:]); err != nil { + return "", fmt.Errorf("generate requestTemplate probe: %w", err) + } + return "gitops-reverser-request-probe-" + hex.EncodeToString(raw[:]), nil +} +// validateRequestTemplate checks that a configured requestTemplate renders, AND that it actually +// puts the request's message in the commit. +// +// The second half is the point. Without it the feature has a hole exactly as bad as the one it was +// designed to avoid: a target could set `requestTemplate: "chore: sync {{.Count}} resources"` and +// every save message would silently vanish — the requester writes a reason, the commit never +// carries it, and the commit is still counted as request-sourced. Framing the message is the whole +// purpose of the field, so a template that drops it is a mistake, not a configuration choice. +// +// It probes the RENDERED OUTPUT rather than scanning the template source. A scan for the literal +// "{{.RequestMessage}}" would reject `{{.RequestMessage | printf "%s"}}`, a template that assigns +// it to a variable first, and every other legitimate spelling — while the probe accepts all of them +// for the right reason: the message reached the commit. +// +// Sample execution cannot prove every branch: a template that drops the message only under, say, +// {{if eq .Count 1}} still passes. That is the same caveat docs/configuration.md already states for +// the other templates, not a new one. +func validateRequestTemplate(config CommitConfig, events []Event) error { + if config.Message.RequestTemplate == "" { + return nil + } + + probe, err := newRequestTemplateProbe() + if err != nil { + return err + } + rendered, err := renderRequestCommitMessage(PendingWrite{ + Kind: PendingWriteCommit, + CommitMessage: probe, + Events: events, + }, config) + if err != nil { + return err + } + // EVERY sample must carry the message through, not merely one: the contract is that a + // requester's reason reaches the commit whatever the window happened to contain. + if !strings.Contains(rendered, probe) { + return errors.New("requestTemplate must render {{.RequestMessage}}: as written it would " + + "drop the CommitRequest's message from the commit. Omit requestTemplate to commit that " + + "message verbatim") + } return nil } diff --git a/internal/git/commit_executor.go b/internal/git/commit_executor.go index 3c872041..0fe3d26c 100644 --- a/internal/git/commit_executor.go +++ b/internal/git/commit_executor.go @@ -30,11 +30,15 @@ func (w *BranchWorker) executePendingWrites( // SHA on push, and a rebase-replay (which re-runs this loop on the retained // writes) refreshes it to the post-rebase hash. for i := range pendingWrites { - created, hash, err := w.executePendingWrite(ctx, repo, worktree, pendingWrites[i]) + created, hash, source, err := w.executePendingWrite(ctx, repo, worktree, pendingWrites[i]) if err != nil { return commitsCreated, err } pendingWrites[i].CommitSHA = hash + // Stamped for the same reason the hash is: publishCommitsForPush runs after the push and + // would otherwise recompute the source from the write, which cannot know that a + // requestTemplate render failed back here at commit time. + pendingWrites[i].committedMessageSource = source commitsCreated += created } @@ -68,6 +72,14 @@ const ( messageResolutionPreRendered // messageResolutionRequest is a message a CommitRequest supplied, used verbatim. messageResolutionRequest + // messageResolutionRequestTemplate is a CommitRequest's message framed by the target's + // requestTemplate. The request's own bytes are never parsed — they ride in as .RequestMessage — + // so the formatting policy is the operator's while the message stays the requester's. + messageResolutionRequestTemplate + // messageResolutionRequestFallback is a requestTemplate that failed to render, so the literal + // was committed instead. Never returned by resolveMessage: it is decided at render time and + // stamped onto the write, because a silent, SUCCESSFUL commit is otherwise invisible. + messageResolutionRequestFallback // messageResolutionReconcileTemplate renders reconcileTemplate from the write's events. messageResolutionReconcileTemplate // messageResolutionLiveTemplate renders liveTemplate for a live window. @@ -80,6 +92,15 @@ func (p PendingWrite) resolveMessage() messageResolution { switch { case p.Kind == PendingWriteResync: return messageResolutionPreRendered + // Framing is bound to the GROUPED kind, not merely to "carries a message". A CommitRequest + // attaches its message to an open live window, which finalizes as PendingWriteCommit, so that + // is the only write requestTemplate describes. An atomic snapshot can also carry a message + // (WriteRequest.CommitMessage), and framing one with the live context would silently reroute + // the documented atomic path through a template written for save requests. + case p.Kind == PendingWriteCommit && + p.CommitMessage != "" && + p.CommitConfig.Message.RequestTemplate != "": + return messageResolutionRequestTemplate case p.CommitMessage != "": return messageResolutionRequest case p.Kind == PendingWriteAtomic: @@ -98,6 +119,10 @@ func (r messageResolution) label() string { switch r { case messageResolutionRequest: return messageSourceCommitRequest + case messageResolutionRequestTemplate: + return messageSourceCommitRequestFramed + case messageResolutionRequestFallback: + return messageSourceCommitRequestFallback case messageResolutionPreRendered, messageResolutionReconcileTemplate: return messageSourceReconcile case messageResolutionLiveTemplate: @@ -112,11 +137,22 @@ func (r messageResolution) label() string { } // messageSource is the commits_total `message_source` label for this write. +// +// It prefers the source stamped at commit time, because that is the only one that knows whether a +// requestTemplate actually rendered. An unstamped write — one that never reached the executor, as +// in a unit test — falls back to what the write itself implies. func (p PendingWrite) messageSource() string { + if p.committedMessageSource != messageResolutionUnsupported { + return p.committedMessageSource.label() + } return p.resolveMessage().label() } -func (p PendingWrite) commitMetadata() (string, *gogit.CommitOptions, error) { +// commitMetadata builds one commit's message and options, and reports the message source it +// actually committed under — which is not always what resolveMessage predicted, because a +// requestTemplate that fails to render falls back to the literal. The caller stamps that back onto +// the write so the commits counter tells the truth; see executePendingWrites. +func (p PendingWrite) commitMetadata() (string, *gogit.CommitOptions, messageResolution, error) { var message string var err error resolution := p.resolveMessage() @@ -126,20 +162,59 @@ func (p PendingWrite) commitMetadata() (string, *gogit.CommitOptions, error) { case messageResolutionRequest: message = p.CommitMessage err = ValidateLiteralCommitMessage(message) + case messageResolutionRequestTemplate: + // Validated on THIS arm too, not just the verbatim one. The literal reaches Git either + // way — framed on success, and by itself on the fallback below — so framing must not + // become a route that smuggles a message past the check the verbatim path enforces. + // The controller validates earlier (commitrequest_controller.go), which makes this + // belt-and-braces rather than reachable today; it is exactly the kind of invariant that + // stops being true once a second producer of PendingWrite appears. + if err = ValidateLiteralCommitMessage(p.CommitMessage); err == nil { + message, resolution = p.framedRequestMessage() + } case messageResolutionReconcileTemplate: message, err = renderReconcileCommitMessageFromEvents(p.Events, p.Target().Name, p.CommitConfig) case messageResolutionLiveTemplate: message, err = renderLiveCommitMessage(p, p.CommitConfig) + case messageResolutionRequestFallback: + // resolveMessage never returns it; only framedRequestMessage produces it, below. + err = fmt.Errorf("unsupported pending write kind %q", p.Kind) case messageResolutionUnsupported: err = fmt.Errorf("unsupported pending write kind %q", p.Kind) default: err = fmt.Errorf("unsupported pending write kind %q", p.Kind) } if err != nil { - return "", nil, err + return "", nil, messageResolutionUnsupported, err } log.Log.V(1).Info("Selected commit message", "source", resolution.label()) - return message, commitOptionsFor(p, p.CommitConfig, p.Signer, time.Now()), nil + return message, commitOptionsFor(p, p.CommitConfig, p.Signer, time.Now()), resolution, nil +} + +// framedRequestMessage renders the target's requestTemplate around an attached CommitRequest's +// message, falling back to that message verbatim if the render fails. +// +// The fallback deviates from liveTemplate, where a render failure fails the write, and the +// deviation is the point: here a correct, non-lossy answer is always in hand, because the literal +// already passed ValidateLiteralCommitMessage at admission. Failing would discard the requester's +// save AND every other author's retained events in the same window, to punish a formatting mistake +// that ValidateCommitConfig should have caught at GitTarget admission and that the fallback makes +// harmless. +// +// It is never silent. The Error log names the target, and the returned resolution moves the commit +// to message_source="commit_request_fallback" — because a fallback is a SUCCESSFUL commit, so no +// condition moves, nothing is refused, and a rate is the only way an operator sees a template that +// has quietly stopped applying. +// +// It returns no error, and cannot: "this failed" is not an outcome here, which is the whole point. +func (p PendingWrite) framedRequestMessage() (string, messageResolution) { + framed, err := renderRequestCommitMessage(p, p.CommitConfig) + if err != nil { + log.Log.Error(err, "requestTemplate failed to render; committing the CommitRequest message verbatim", + "gitTarget", p.Target().Namespace+"/"+p.Target().Name) + return p.CommitMessage, messageResolutionRequestFallback + } + return framed, messageResolutionRequestTemplate } func (w *BranchWorker) executePendingWrite( @@ -147,20 +222,21 @@ func (w *BranchWorker) executePendingWrite( repo *gogit.Repository, worktree *gogit.Worktree, pendingWrite PendingWrite, -) (int, plumbing.Hash, error) { +) (int, plumbing.Hash, messageResolution, error) { switch pendingWrite.Kind { case PendingWriteResync: // Resync writes never carry a CommitRequest, so their commit hash is unused; // report ZeroHash to keep the per-write SHA bookkeeping uniform. created, err := w.executeResyncPendingWrite(ctx, repo, worktree, pendingWrite) - return created, plumbing.ZeroHash, err + return created, plumbing.ZeroHash, messageResolutionPreRendered, err case PendingWriteCommit, PendingWriteAtomic: default: - return 0, plumbing.ZeroHash, fmt.Errorf("unsupported pending write kind %q", pendingWrite.Kind) + return 0, plumbing.ZeroHash, messageResolutionUnsupported, + fmt.Errorf("unsupported pending write kind %q", pendingWrite.Kind) } if len(pendingWrite.Events) == 0 { - return 0, plumbing.ZeroHash, nil + return 0, plumbing.ZeroHash, messageResolutionUnsupported, nil } target := pendingWrite.Target() @@ -170,25 +246,27 @@ func (w *BranchWorker) executePendingWrite( encryptionPath, target.EncryptionConfig, ); err != nil { - return 0, plumbing.ZeroHash, fmt.Errorf("configure secret encryptor: %w", err) + return 0, plumbing.ZeroHash, messageResolutionUnsupported, + fmt.Errorf("configure secret encryptor: %w", err) } anyChanges, err := w.applyPendingWriteEvents(ctx, repo, worktree, pendingWrite.Events, pendingWrite.Targets) if err != nil { - return 0, plumbing.ZeroHash, err + return 0, plumbing.ZeroHash, messageResolutionUnsupported, err } if !anyChanges { - return 0, plumbing.ZeroHash, nil + return 0, plumbing.ZeroHash, messageResolutionUnsupported, nil } - commitMessage, commitOptions, err := pendingWrite.commitMetadata() + commitMessage, commitOptions, source, err := pendingWrite.commitMetadata() if err != nil { - return 0, plumbing.ZeroHash, err + return 0, plumbing.ZeroHash, messageResolutionUnsupported, err } hash, err := worktree.Commit(commitMessage, commitOptions) if err != nil { - return 0, plumbing.ZeroHash, fmt.Errorf("failed to create commit: %w", err) + return 0, plumbing.ZeroHash, messageResolutionUnsupported, + fmt.Errorf("failed to create commit: %w", err) } log.FromContext(ctx).Info( @@ -198,7 +276,7 @@ func (w *BranchWorker) executePendingWrite( "message", commitMessage, ) - return 1, hash, nil + return 1, hash, source, nil } func (w *BranchWorker) applyPendingWriteEvents( diff --git a/internal/git/commit_executor_test.go b/internal/git/commit_executor_test.go index c68a22c0..eb8f6c1e 100644 --- a/internal/git/commit_executor_test.go +++ b/internal/git/commit_executor_test.go @@ -109,7 +109,7 @@ func TestExecutor_GroupedSingleEvent_UsesLiveTemplate(t *testing.T) { CommitConfig: config, } - message, options, err := pendingWrite.commitMetadata() + message, options, _, err := pendingWrite.commitMetadata() require.NoError(t, err) assert.Equal(t, "group: alice changed 1", message) assert.Equal(t, "alice", options.Author.Name) @@ -129,7 +129,7 @@ func TestExecutor_GroupedMultiEvent_UsesLiveTemplate(t *testing.T) { CommitConfig: config, } - message, options, err := pendingWrite.commitMetadata() + message, options, _, err := pendingWrite.commitMetadata() require.NoError(t, err) assert.Equal(t, "group: alice 2 team-a", message) assert.Equal(t, "alice", options.Author.Name) @@ -152,7 +152,7 @@ func TestExecutor_AtomicUnit_UsesReconcileMessage(t *testing.T) { GitTargetNamespace: "default", } - message, options, err := pendingWrite.commitMetadata() + message, options, _, err := pendingWrite.commitMetadata() require.NoError(t, err) assert.Equal(t, "reconcile: 2 team-a", message) assert.Equal(t, DefaultCommitterName, options.Author.Name) @@ -167,7 +167,7 @@ func TestExecutor_NoOpUnit_SkipsCommit(t *testing.T) { headBefore, err := repo.Head() require.NoError(t, err) - created, hash, err := worker.executePendingWrite(context.Background(), repo, worktree, PendingWrite{ + created, hash, _, err := worker.executePendingWrite(context.Background(), repo, worktree, PendingWrite{ Kind: PendingWriteCommit, Events: []Event{event}, CommitConfig: ResolveCommitConfig(nil), @@ -210,7 +210,7 @@ func TestExecutor_AppliesEncryptionFromPendingWrite_NotFromWorker(t *testing.T) }, } - created, hash, err := worker.executePendingWrite(context.Background(), repo, worktree, pendingWrite) + created, hash, _, err := worker.executePendingWrite(context.Background(), repo, worktree, pendingWrite) require.NoError(t, err) assert.Equal(t, 1, created) assert.False(t, hash.IsZero(), "a committed write reports its commit hash") @@ -246,7 +246,7 @@ func TestCommitMetadata_ResyncRenderedMessageIsNotHeldToTheLiteralRequestContrac CommitMessage: rendered, } - message, options, err := pendingWrite.commitMetadata() + message, options, _, err := pendingWrite.commitMetadata() require.NoError(t, err) assert.Equal(t, rendered, message) assert.NotNil(t, options) @@ -300,7 +300,7 @@ func TestMessageSource_MatchesTheMessageActuallyRendered(t *testing.T) { assert.Equal(t, tc.want, write.messageSource()) - message, _, err := write.commitMetadata() + message, _, _, err := write.commitMetadata() require.NoError(t, err) if tc.message != "" { assert.Equal(t, tc.message, message) diff --git a/internal/git/commit_metadata_fields_test.go b/internal/git/commit_metadata_fields_test.go index 77baaa31..3d566717 100644 --- a/internal/git/commit_metadata_fields_test.go +++ b/internal/git/commit_metadata_fields_test.go @@ -32,7 +32,7 @@ func labeledDeploymentEvent(name, namespace string, labels map[string]string) Ev } func TestBuildLiveCommitMessageData_CarriesKindAndLabels(t *testing.T) { - data := buildLiveCommitMessageData("someone", "target", []Event{ + data := buildLiveCommitMessageData("someone", "target", "", []Event{ labeledDeploymentEvent("api", "prod", map[string]string{"team": "payments"}), }) @@ -57,7 +57,7 @@ func TestBuildLiveCommitMessageData_ClusterScopedRendersTheSentinel(t *testing.T event := labeledDeploymentEvent("admin", "", nil) event.Identifier = types.NewResourceIdentifier("rbac.authorization.k8s.io", "v1", "clusterroles", "", "admin") - data := buildLiveCommitMessageData("someone", "target", []Event{event}) + data := buildLiveCommitMessageData("someone", "target", "", []Event{event}) if got := data.Resources[0].Namespace; got != types.ClusterScopeSegment { t.Errorf("Namespace = %q, want the %q sentinel, so a template need not guard it", @@ -72,7 +72,7 @@ func TestBuildLiveCommitMessageData_DeleteHasNoObjectMetadata(t *testing.T) { event.Object = nil event.Operation = "DELETE" - ref := buildLiveCommitMessageData("someone", "target", []Event{event}).Resources[0] + ref := buildLiveCommitMessageData("someone", "target", "", []Event{event}).Resources[0] if ref.Kind != "" || ref.Labels != nil { t.Errorf("Kind = %q, Labels = %v, want both empty for a DELETE", ref.Kind, ref.Labels) @@ -84,7 +84,7 @@ func TestBuildLiveCommitMessageData_DeleteHasNoObjectMetadata(t *testing.T) { // A commit is 1:n, so a label is a SET here where placement reads a single value. func TestLiveCommitMessageData_LabelValues(t *testing.T) { - data := buildLiveCommitMessageData("someone", "target", []Event{ + data := buildLiveCommitMessageData("someone", "target", "", []Event{ labeledDeploymentEvent("api", "prod", map[string]string{"team": "payments"}), labeledDeploymentEvent("web", "prod", map[string]string{"team": "storefront"}), labeledDeploymentEvent("cache", "prod", map[string]string{"team": "payments"}), @@ -112,7 +112,7 @@ func TestLiveCommitMessageData_LabelValues(t *testing.T) { // The subject-line case: one shared value names the whole commit. func TestLiveCommitMessageData_LabelValue_AgreedValue(t *testing.T) { - data := buildLiveCommitMessageData("someone", "target", []Event{ + data := buildLiveCommitMessageData("someone", "target", "", []Event{ labeledDeploymentEvent("api", "prod", map[string]string{"team": "payments"}), labeledDeploymentEvent("cache", "prod", map[string]string{"team": "payments"}), }) @@ -137,7 +137,7 @@ func TestLiveCommitMessageData_LabelValue_PartiallyLabeledCommitNamesNoOne(t *te {"a different label", labeledDeploymentEvent("other", "prod", map[string]string{"squad": "payments"})}, } { t.Run(tc.name, func(t *testing.T) { - data := buildLiveCommitMessageData("someone", "target", []Event{labeled, tc.other}) + data := buildLiveCommitMessageData("someone", "target", "", []Event{labeled, tc.other}) if v := data.LabelValue("team"); v != "" { t.Errorf("LabelValue = %q, want empty: %q carries no team, so the commit is not one team's", @@ -154,7 +154,7 @@ func TestLiveCommitMessageData_LabelValue_DeleteLeavesTheCommitUnnamed(t *testin deleted.Object = nil deleted.Operation = "DELETE" - data := buildLiveCommitMessageData("someone", "target", []Event{ + data := buildLiveCommitMessageData("someone", "target", "", []Event{ labeledDeploymentEvent("api", "prod", map[string]string{"team": "payments"}), deleted, }) diff --git a/internal/git/literal_message_test.go b/internal/git/literal_message_test.go index 48924d94..f2c2cd98 100644 --- a/internal/git/literal_message_test.go +++ b/internal/git/literal_message_test.go @@ -43,14 +43,14 @@ func TestCommitMetadata_LiteralPrecedenceAndPreservation(t *testing.T) { CommitConfig: ResolveCommitConfig(nil)} p.CommitConfig.Message.LiveTemplate = "{{ invalid" p.CommitConfig.Message.ReconcileTemplate = "{{ invalid" - actual, options, err := p.commitMetadata() + actual, options, _, err := p.commitMetadata() require.NoError(t, err) assert.Equal(t, message, actual) assert.Equal(t, DefaultCommitterName, options.Committer.Name) } } p := PendingWrite{Kind: PendingWriteCommit, CommitMessage: " \n ", CommitConfig: ResolveCommitConfig(nil)} - _, _, err := p.commitMetadata() + _, _, _, err := p.commitMetadata() require.ErrorContains(t, err, "non-whitespace") } diff --git a/internal/git/open_window.go b/internal/git/open_window.go index 46dc278b..b32813c8 100644 --- a/internal/git/open_window.go +++ b/internal/git/open_window.go @@ -103,7 +103,14 @@ 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. -func buildLiveCommitMessageData(author, gitTarget string, events []Event) LiveCommitMessageData { +// +// 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 +// reaching the commit is the audit property the whole CommitRequest contract rests on. +func buildLiveCommitMessageData( + author, gitTarget, requestMessage string, + events []Event, +) LiveCommitMessageData { operations := make(map[string]int, groupedCommitOperationKinds) resources := make([]ResourceRef, 0, len(events)) for _, e := range events { @@ -129,10 +136,11 @@ func buildLiveCommitMessageData(author, gitTarget string, events []Event) LiveCo }) } return LiveCommitMessageData{ - Author: author, - GitTarget: gitTarget, - Count: len(events), - Operations: operations, - Resources: resources, + Author: author, + GitTarget: gitTarget, + Count: len(events), + Operations: operations, + Resources: resources, + RequestMessage: requestMessage, } } diff --git a/internal/git/pending_writes.go b/internal/git/pending_writes.go index 7f47a2c3..729673ce 100644 --- a/internal/git/pending_writes.go +++ b/internal/git/pending_writes.go @@ -335,6 +335,18 @@ const ( // verbatim. It counts commits that USED such a message, not CommitRequests: a request // omitting spec.message takes the target's liveTemplate and counts as live. messageSourceCommitRequest = "commit_request" + // messageSourceCommitRequestFramed is a CommitRequest message rendered through the target's + // requestTemplate. Split from commit_request rather than folded into it, even though the text + // originated with the request either way: the difference is not HOW the text was produced (the + // distinction reconcile deliberately collapses, below) but WHETHER the operator's configured + // framing applied at all, which is a question an operator actually asks. + messageSourceCommitRequestFramed = "commit_request_framed" + // messageSourceCommitRequestFallback is a requestTemplate that failed to render, so the + // request's message was committed verbatim instead. It is the ONLY signal for that: the commit + // succeeds, nothing is refused, and no condition moves, so without a counter a template that + // has quietly stopped applying looks exactly like one that was never configured. Alert on its + // rate; see docs/interpreting-metrics.md. + messageSourceCommitRequestFallback = "commit_request_fallback" // messageSourceLive is a live window rendered through the target's liveTemplate. messageSourceLive = "live" // messageSourceReconcile is an atomic snapshot or a resync rendered through diff --git a/internal/git/request_template_test.go b/internal/git/request_template_test.go new file mode 100644 index 00000000..5cb6f43b --- /dev/null +++ b/internal/git/request_template_test.go @@ -0,0 +1,269 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +const requestBodyTemplate = "{{.RequestMessage}}\n\n" + + "{{range .Resources -}}\n- [{{.Operation}}] {{.Resource}}/{{.Name}}\n{{end -}}" + +func requestConfig(requestTemplate string) CommitConfig { + return ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ + RequestTemplate: requestTemplate, + }) +} + +func requestWrite(message string, config CommitConfig) PendingWrite { + return PendingWrite{ + Kind: PendingWriteCommit, + CommitMessage: message, + CommitConfig: config, + Events: []Event{{ + Operation: "UPDATE", + Identifier: types.ResourceIdentifier{ + Group: "apps", Version: "v1", Resource: "deployments", + Namespace: "prod", Name: "api", + }, + UserInfo: UserInfo{Username: "alice"}, + GitTargetName: "platform", + GitTargetNamespace: "default", + }}, + Targets: map[pendingTargetKey]ResolvedTargetMetadata{ + {Name: "platform", Namespace: "default"}: {Name: "platform", Namespace: "default"}, + }, + } +} + +// The gap this feature closes: the override used to REPLACE the template, so supplying a save +// message cost you the resource body liveTemplate would have produced. "Why I saved" and "what was +// saved" were mutually exclusive. +func TestFramedRequestMessage_ComposesTheReasonWithWhatWasSaved(t *testing.T) { + config := requestConfig(requestBodyTemplate) + write := requestWrite("fix(api): correct the service port", config) + + message, resolution := write.framedRequestMessage() + + assert.Equal(t, messageResolutionRequestTemplate, resolution) + assert.Equal(t, "fix(api): correct the service port\n\n- [UPDATE] deployments/api\n", message) +} + +// The contract PR 1 of this series must not break: the request's bytes reach the commit unaltered. +// A target with no requestTemplate commits exactly what was supplied — no trimming, no framing, no +// reformatting — which is what makes a CommitRequest auditable against the commit it produced. +func TestCommitMetadata_WithoutARequestTemplateTheLiteralIsCommittedByteForByte(t *testing.T) { + literal := " fix(api): correct the service port \n\n Route traffic to the API container. " + write := requestWrite(literal, ResolveCommitConfig(nil)) + + message, _, resolution, err := write.commitMetadata() + require.NoError(t, err) + + assert.Equal(t, literal, message, "an unframed request message must not be altered at all") + assert.Equal(t, messageResolutionRequest, resolution) + assert.Equal(t, messageSourceCommitRequest, resolution.label()) +} + +// A requestTemplate that fails at render time must not cost the window. The literal already passed +// admission validation, so a correct answer is always in hand; failing would discard the requester's +// save AND every other author's retained events, to punish a formatting mistake. +// +// It is not silent: the commit moves to its own message_source, which is the only signal an +// operator gets for a template that has quietly stopped applying. +func TestFramedRequestMessage_RenderFailureFallsBackToTheLiteralAndSaysSo(t *testing.T) { + // missingkey=error: Labels carries no "team" on this sample, so this renders at admission for a + // labelled resource and dies here for one without. + config := requestConfig("{{.RequestMessage}} {{(index .Resources 0).Labels.team}}") + write := requestWrite("fix(api): correct the service port", config) + + // No error return at all: "the template failed" is not an outcome this function can report, + // which is exactly the guarantee the window depends on. + message, resolution := write.framedRequestMessage() + + assert.Equal(t, "fix(api): correct the service port", message) + assert.Equal(t, messageResolutionRequestFallback, resolution) + assert.Equal(t, messageSourceCommitRequestFallback, resolution.label()) +} + +// The framed and fallback arms must stay distinguishable in commits_total. Folding them together +// would leave "the template works" and "the template silently stopped applying" reading identically. +func TestMessageSource_SeparatesFramedFromFallbackAndPlainRequest(t *testing.T) { + framedConfig := requestConfig(requestBodyTemplate) + + assert.Equal(t, messageSourceCommitRequest, + requestWrite("save", ResolveCommitConfig(nil)).messageSource()) + assert.Equal(t, messageSourceCommitRequestFramed, + requestWrite("save", framedConfig).messageSource()) + + // Stamped at commit time, which is the only place the fallback is known. + stamped := requestWrite("save", framedConfig) + stamped.committedMessageSource = messageResolutionRequestFallback + assert.Equal(t, messageSourceCommitRequestFallback, stamped.messageSource()) +} + +// Precedence: a configured requestTemplate outranks the verbatim arm, and a window with no request +// message is untouched by the field entirely. +func TestResolveMessage_RequestTemplateOutranksTheLiteralArm(t *testing.T) { + framedConfig := requestConfig(requestBodyTemplate) + + assert.Equal(t, messageResolutionRequestTemplate, + requestWrite("save", framedConfig).resolveMessage()) + assert.Equal(t, messageResolutionRequest, + requestWrite("save", ResolveCommitConfig(nil)).resolveMessage()) + assert.Equal(t, messageResolutionLiveTemplate, + requestWrite("", framedConfig).resolveMessage(), + "a window no request attached to renders liveTemplate, template configured or not") +} + +// The rule that makes the audit property hold rather than merely hoped for. Without it a target +// could frame away the requester's reason entirely and the commit would still count as +// request-sourced. +func TestValidateCommitConfig_RejectsARequestTemplateThatDropsTheMessage(t *testing.T) { + err := ValidateCommitConfig(requestConfig("chore: sync {{.Count}} resources")) + + require.Error(t, err) + assert.Contains(t, err.Error(), "requestTemplate must render {{.RequestMessage}}") +} + +// The accept half, and the reason the check probes the RENDERED OUTPUT instead of scanning the +// template source: every one of these is a legitimate way to put the message in the commit, and a +// source scan for the literal "{{.RequestMessage}}" would reject all but the first. +func TestValidateCommitConfig_AcceptsEveryHonestSpellingOfRequestMessage(t *testing.T) { + for name, template := range map[string]string{ + "direct": "{{.RequestMessage}}", + "piped": `{{.RequestMessage | printf "%s"}}`, + "via variable": "{{$m := .RequestMessage}}chore: save\n\n{{$m}}", + "guarded": "{{if .RequestMessage}}{{.RequestMessage}}{{end}}", + "with a body": requestBodyTemplate, + "with a prefix": "save: {{.RequestMessage}}", + } { + t.Run(name, func(t *testing.T) { + assert.NoError(t, ValidateCommitConfig(requestConfig(template))) + }) + } +} + +func TestValidateCommitConfig_RejectsAnUnparseableRequestTemplate(t *testing.T) { + err := ValidateCommitConfig(requestConfig("{{.RequestMessage")) + + require.Error(t, err) + assert.Contains(t, err.Error(), "parse request commit template") +} + +// missingkey=error makes a label a template names but a resource does not carry a render failure. +// Catching it at GitTarget admission is the whole reason validation renders samples: the +// alternative is a commit that falls back months later, for a fault nobody was told about. +// +// It only holds because requestTemplate is validated against the SAME window shapes as +// liveTemplate. A single sample would not catch this: the labelled sample carries "team", so the +// fault appears only on the entries that carry no object at all — which is exactly the shape a +// DELETE has in production, since the object is gone by the time the event is built. +func TestValidateCommitConfig_RejectsARequestTemplateThatReadsAMissingLabel(t *testing.T) { + err := ValidateCommitConfig(requestConfig( + "{{.RequestMessage}}{{range .Resources}} {{.Labels.team}}{{end}}")) + + require.Error(t, err) + assert.Contains(t, err.Error(), "execute request commit template") + + // The accessor form renders empty instead of failing, and must still be accepted — the same + // escape hatch liveTemplate documents. + assert.NoError(t, ValidateCommitConfig(requestConfig( + `{{.RequestMessage}}{{range .Resources}} {{.Label "team"}}{{end}}`))) +} + +// An unset requestTemplate is "do not frame", not "use a standard framing", so it must have no +// built-in default the way the other two templates do. +func TestResolveCommitConfig_RequestTemplateHasNoDefault(t *testing.T) { + assert.Empty(t, ResolveCommitConfig(nil).Message.RequestTemplate) + require.NoError(t, ValidateCommitConfig(ResolveCommitConfig(nil))) + + overlaid := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ + RequestTemplate: " " + requestBodyTemplate + " ", + }) + assert.Equal(t, requestBodyTemplate, overlaid.Message.RequestTemplate, "overlay trims like the others") +} + +// Framing must not become a route that smuggles a message past the check the verbatim arm +// enforces. The controller validates earlier, so this is not reachable through the normal flow — +// it is the invariant that has to survive a second producer of PendingWrite appearing. +func TestCommitMetadata_FramingStillValidatesTheLiteralMessage(t *testing.T) { + config := requestConfig(requestBodyTemplate) + + for name, message := range map[string]string{ + "a control character": "fix(api): correct\tthe service port", + "whitespace only": " \n ", + } { + t.Run(name, func(t *testing.T) { + _, _, _, err := requestWrite(message, config).commitMetadata() + require.Error(t, err, "a framed message must face the same literal check as a verbatim one") + + // Same verdict on both arms, so framing cannot change what is accepted. + _, _, _, unframedErr := requestWrite(message, ResolveCommitConfig(nil)).commitMetadata() + require.Error(t, unframedErr) + }) + } + + // And the valid case still renders, so the check gates nothing it should not. + message, _, resolution, err := requestWrite("fix(api): correct the service port", config).commitMetadata() + require.NoError(t, err) + assert.Equal(t, messageResolutionRequestTemplate, resolution) + assert.Contains(t, message, "fix(api): correct the service port") +} + +// A fixed sentinel would let a template satisfy the check by emitting the sentinel itself, never +// referencing .RequestMessage. The probe is minted fresh per validation precisely so that the only +// way to pass is to render the message. +func TestValidateCommitConfig_RejectsATemplateThatFakesTheProbe(t *testing.T) { + for name, template := range map[string]string{ + "the old fixed sentinel": "", + "the probe prefix": "gitops-reverser-request-probe-00000000000000000000000000000000", + } { + t.Run(name, func(t *testing.T) { + err := ValidateCommitConfig(requestConfig(template)) + + require.Error(t, err) + assert.Contains(t, err.Error(), "requestTemplate must render {{.RequestMessage}}") + }) + } +} + +// Two validations of the same template must not be able to agree by accident on a constant. +func TestNewRequestTemplateProbe_IsFreshEachTime(t *testing.T) { + first, err := newRequestTemplateProbe() + require.NoError(t, err) + second, err := newRequestTemplateProbe() + require.NoError(t, err) + + assert.NotEqual(t, first, second) + assert.NotEmpty(t, first) +} + +// requestTemplate describes the commit a CommitRequest's message produces, and a CommitRequest +// attaches to a live window — which finalizes as PendingWriteCommit. An atomic snapshot can also +// carry a message, and it must keep the behaviour it had before requestTemplate existed: committed +// verbatim, never framed with a live context it was not written for. +func TestResolveMessage_AtomicWriteIsNeverFramed(t *testing.T) { + config := requestConfig(requestBodyTemplate) + + atomic := requestWrite("chore: snapshot", config) + atomic.Kind = PendingWriteAtomic + assert.Equal(t, messageResolutionRequest, atomic.resolveMessage(), + "an atomic write with a message stays verbatim even when requestTemplate is configured") + assert.Equal(t, messageSourceCommitRequest, atomic.messageSource()) + + // And with no message it still renders reconcileTemplate, as documented. + plainAtomic := requestWrite("", config) + plainAtomic.Kind = PendingWriteAtomic + assert.Equal(t, messageResolutionReconcileTemplate, plainAtomic.resolveMessage()) + + // A resync is untouched either way. + resync := requestWrite("pre-rendered", config) + resync.Kind = PendingWriteResync + assert.Equal(t, messageResolutionPreRendered, resync.resolveMessage()) +} diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index 579001ae..28002c45 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -301,7 +301,7 @@ func (w *BranchWorker) executeResyncPendingWrite( return 0, err } pendingWrite.CommitMessage = rendered - message, options, err := pendingWrite.commitMetadata() + message, options, _, err := pendingWrite.commitMetadata() if err != nil { return 0, err } diff --git a/internal/git/types.go b/internal/git/types.go index 605a1c37..789c4cfb 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -305,6 +305,12 @@ type PendingWrite struct { // resolved Committed (with CommitSHA) once this write is pushed. It rides the write through the // push cooldown and the conflict rebase-replay, so the result follows the data. CommitRequest *commitRequestID + // committedMessageSource is the message source this write actually committed under, stamped by + // executePendingWrites the way CommitSHA is and for the same reason: publishCommitsForPush runs + // after the push, and recomputing the source from the write cannot know that a requestTemplate + // render failed at commit time. Zero means "not stamped", so messageSource() recomputes. + committedMessageSource messageResolution + // CommitSHA is the hash of the commit this write created, captured in // executePendingWrite and refreshed when the write is re-executed on a // rebase-replay (so it is never a stale pre-rebase hash). Zero when the write @@ -588,6 +594,9 @@ type CommitterConfig struct { type CommitMessageConfig struct { LiveTemplate string ReconcileTemplate string + // RequestTemplate frames a CommitRequest's message. Empty — the default — commits that + // message verbatim, which is the behaviour every target had before the field existed. + RequestTemplate string } // ReconcileCommitMessageData is the template context for reconcile commit messages. @@ -690,6 +699,14 @@ type LiveCommitMessageData struct { // Resources is the per-resource list, deduplicated by file path so the // final state is what's being committed. Resources []ResourceRef + // RequestMessage is the message an attached CommitRequest supplied, unaltered. It is empty for + // every window no request attached to, which is the ordinary case and why liveTemplate may + // reference it freely. + // + // It lives on this struct rather than on a parallel request-only context for the reason + // ResourceRef's own comment gives about the placement vocabulary: a reader should not have to + // learn two vocabularies to describe one commit. One sample builder then serves both renders. + RequestMessage string } // LabelValues is the sorted, distinct set of values this commit's resources carry for one @@ -790,5 +807,11 @@ func (c CommitConfig) WithTargetMessage(spec *v1alpha3.CommitMessageSpec) Commit if reconcileTemplate := strings.TrimSpace(spec.ReconcileTemplate); reconcileTemplate != "" { c.Message.ReconcileTemplate = reconcileTemplate } + // No built-in default to fall back to, unlike the two above: an unset requestTemplate is not + // "use the standard framing", it is "do not frame at all", and that has to stay distinguishable + // from a configured one or every existing target would start reformatting its save messages. + if requestTemplate := strings.TrimSpace(spec.RequestTemplate); requestTemplate != "" { + c.Message.RequestTemplate = requestTemplate + } return c } diff --git a/test/e2e/inplace_edit_e2e_test.go b/test/e2e/inplace_edit_e2e_test.go index e2a68f55..9d946d01 100644 --- a/test/e2e/inplace_edit_e2e_test.go +++ b/test/e2e/inplace_edit_e2e_test.go @@ -390,30 +390,85 @@ func renderInPlaceFixtureFolder(fixtureRoot, namespace string) string { return rendered } +// seedRenderedFolderIntoRepo pushes a fixture folder straight into the target branch, the way a +// human editing the repo by hand would. +// +// It RETRIES, because by the time most callers reach it the GitTarget is healthy and actively +// mirroring into this same branch. The seed reads the remote tip, builds a commit on it and pushes +// with the remote's compare-and-swap in force, so any controller commit landing in that window +// rejects the push: +// +// ! [remote rejected] HEAD -> main (incorrect old value provided) +// +// Losing that race is expected and says nothing about the behaviour under test, so each attempt +// rebuilds the commit on the new tip rather than failing the spec. The window is small but real: +// it was reported as an intermittent failure of the unsupported-folder spec, and the helper is +// shared by six spec files, so one unretried push is a flake source for all of them. func seedRenderedFolderIntoRepo(repo *RepoArtifacts, namespace, renderedFolder, gitPath string) { GinkgoHelper() configureRepoOriginWithCredentials(repo, namespace) - mustGit := func(args ...string) { + Eventually(func() error { + return attemptSeedRenderedFolder(repo, renderedFolder, gitPath) + }, seedPushTimeout, seedPushInterval).Should(Succeed(), + "seeding %q kept losing the push race with the controller's own commits", gitPath) +} + +const ( + seedPushTimeout = 60 * time.Second + seedPushInterval = 2 * time.Second +) + +// attemptSeedRenderedFolder is one seed attempt, from the CURRENT remote tip. +// +// It returns errors rather than asserting, because every step of it is retried: an Expect in here +// would fail the spec on the first lost race instead of letting Eventually rebuild on the new tip. +func attemptSeedRenderedFolder(repo *RepoArtifacts, renderedFolder, gitPath string) error { + runGit := func(args ...string) error { out, gitErr := gitRun(repo.CheckoutDir, args...) - Expect(gitErr).NotTo(HaveOccurred(), fmt.Sprintf("git %s: %s", strings.Join(args, " "), out)) + if gitErr != nil { + return fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), gitErr, out) + } + return nil } + // Re-fetch on EVERY attempt: the whole point of retrying is to rebuild on the tip that beat us. if _, err := gitRun(repo.CheckoutDir, "fetch", "origin", "main"); err == nil { - mustGit("checkout", "-B", "main", "origin/main") - mustGit("reset", "--hard", "origin/main") + if err := runGit("checkout", "-B", "main", "origin/main"); err != nil { + return err + } + if err := runGit("reset", "--hard", "origin/main"); err != nil { + return err + } } else { - mustGit("checkout", "--orphan", "main") + if err := runGit("checkout", "--orphan", "main"); err != nil { + return err + } _, _ = gitRun(repo.CheckoutDir, "rm", "-rf", ".") } dest := filepath.Join(repo.CheckoutDir, gitPath) - Expect(os.RemoveAll(dest)).To(Succeed()) - Expect(copyFixtureDir(renderedFolder, dest)).To(Succeed()) + if err := os.RemoveAll(dest); err != nil { + return fmt.Errorf("clear %s: %w", gitPath, err) + } + if err := copyFixtureDir(renderedFolder, dest); err != nil { + return fmt.Errorf("copy fixture into %s: %w", gitPath, err) + } - mustGit("add", gitPath) - mustGit("commit", "-m", "e2e: seed manifest folder fixture") - mustGit("push", "origin", "HEAD:main") + if err := runGit("add", gitPath); err != nil { + return err + } + // A retry that lands on a tip already carrying this exact content stages nothing, and + // `git commit` with no changes is an error. That is success: the fixture is in Git, which is + // all the caller asked for. + if staged, err := gitRun(repo.CheckoutDir, "diff", "--cached", "--quiet"); err == nil { + _ = staged + return nil + } + if err := runGit("commit", "-m", "e2e: seed manifest folder fixture"); err != nil { + return err + } + return runGit("push", "origin", "HEAD:main") } func copyFixtureDir(src, dst string) error {