diff --git a/internal/controllers/repo/repo.go b/internal/controllers/repo/repo.go index faeb28f..83c34a0 100644 --- a/internal/controllers/repo/repo.go +++ b/internal/controllers/repo/repo.go @@ -184,7 +184,24 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler if !isTargetRepoSynced { e.log.Debug("Target commit not found in target remote repository", "commitId", cr.Status.TargetCommitId, "branch", cr.Status.TargetBranch) if !cr.Spec.EnableUpdate { - return reconciler.ExternalObservation{}, e.failSync(ctx, cr, fmt.Errorf("target commit %s not found on branch %s while enableUpdate is false", cr.Status.TargetCommitId, cr.Status.TargetBranch)) + // Stays an error on purpose. The resource genuinely cannot be reconciled and a human + // has to choose between re-pinning and allowing updates, so surfacing it as + // Synced=False/ReconcileError is right — TC12 and TC13 pin that contract deliberately, + // and TC13 covers the auto-heal once enableUpdate is flipped. + // + // What WAS wrong is the wording. "target commit %s not found on branch %s while + // enableUpdate is false" describes what the check found and reads as though the commit + // was pinned wrong. Usually it was not: the commit was discarded along with the history + // that contained it, when the target repository was recreated or force-pushed. Nine + // Repo CRs hit this simultaneously on one such event, and the message sent the reader + // looking for nine bad pins (#23). + // + // Note this is NOT the #22 situation, despite both being reached from this function. + // There the provider refused on an ambiguity nothing would ever resolve, so the + // resource was stranded with no path back. Here there is a real decision for a person + // to make, and reporting it until they make it is correct. + return reconciler.ExternalObservation{}, e.failSync(ctx, cr, unreachableTargetCommitError( + cr.Status.TargetCommitId, cr.Status.TargetBranch, cr.Spec.ToRepo.Url)) } return reconciler.ExternalObservation{ ResourceExists: true, diff --git a/internal/controllers/repo/unreachable_commit.go b/internal/controllers/repo/unreachable_commit.go new file mode 100644 index 0000000..a791b73 --- /dev/null +++ b/internal/controllers/repo/unreachable_commit.go @@ -0,0 +1,31 @@ +package repo + +import ( + "fmt" +) + +// unreachableTargetCommitError reports a recorded target commit that the target branch can no +// longer reach, when spec.enableUpdate forbids doing anything about it. +// +// The wording is the point. The previous message — "target commit X not found on branch main while +// enableUpdate is false" — describes what the check found, and reads as though the commit was +// pinned wrong. Usually it was not: the commit was discarded along with the history that contained +// it, when the target repository was recreated or force-pushed. Nine Repo CRs hit this at once on a +// single such event, and the message sent the reader looking for nine separate bad pins (#23). +// +// It stays an error. There is a real decision here for a person to make, so reporting it until they +// make it is correct — unlike #22, where the provider refused on an ambiguity nothing would ever +// resolve and the resource was stranded with no path back. +// +// What the message must carry: which commit and target (so the resource is identifiable), the CAUSE +// (so the reader does not go hunting for a mistake that was not made), and both remedies. Naming +// only enableUpdate would push operators toward allowing overwrites they may not want. +func unreachableTargetCommitError(commitID, branch, toRepoURL string) error { + return fmt.Errorf( + "target commit %s is no longer reachable from branch %s of %s, and spec.enableUpdate is false. "+ + "This usually means the target repository was recreated or its history rewritten, which "+ + "discards the commit rather than invalidating it — it does not mean the commit was pinned "+ + "wrong. Set enableUpdate to true to let the provider re-push, or recreate this resource to "+ + "pin the current history.", + commitID, branch, toRepoURL) +} diff --git a/internal/controllers/repo/unreachable_commit_test.go b/internal/controllers/repo/unreachable_commit_test.go new file mode 100644 index 0000000..7bed127 --- /dev/null +++ b/internal/controllers/repo/unreachable_commit_test.go @@ -0,0 +1,58 @@ +package repo + +import ( + "strings" + "testing" +) + +// #23. The message must name the CAUSE and both remedies, not just the symptom. +// +// "target commit X not found on branch main while enableUpdate is false" reads as a bad pin. In the +// incident that prompted this, nothing was pinned wrong — one target repository was recreated, +// orphaning the recorded commits of nine resources at once. A message that implies the wrong remedy +// sends the reader to fix something that is not broken. +// +// The error itself is deliberately preserved: a person has to choose between re-pinning and +// allowing updates, and TC12/TC13 pin that contract. +func TestUnreachableTargetCommitErrorExplainsTheCause(t *testing.T) { + const ( + commit = "131ceb6bc" + branch = "main" + url = "https://github.com/krateo-blueprints/demo-service-catalog.git" + ) + msg := unreachableTargetCommitError(commit, branch, url).Error() + + // Identifiers, so the operator knows which resource and which target. + for _, want := range []string{commit, branch, url} { + if !strings.Contains(msg, want) { + t.Errorf("message must identify %q, got: %s", want, msg) + } + } + + // The cause. Without it the reader assumes a mistaken pin. + for _, want := range []string{"recreated", "history"} { + if !strings.Contains(msg, want) { + t.Errorf("message must explain the cause (%q), got: %s", want, msg) + } + } + + // Both remedies. Naming only the flag pushes operators toward allowing overwrites they may not + // want; re-creating the resource re-pins to the current history instead. + for _, want := range []string{"enableUpdate", "recreate"} { + if !strings.Contains(msg, want) { + t.Errorf("message must offer the %q remedy, got: %s", want, msg) + } + } +} + +// TC12 asserts the Synced message contains "target commit". That substring is load-bearing for the +// integration suite, and it is also what makes the error greppable across a fleet — nine resources +// failing for one reason should be findable as one string. +func TestUnreachableTargetCommitErrorKeepsTheGreppablePrefix(t *testing.T) { + msg := unreachableTargetCommitError("abc123", "main", "https://example.com/repo.git").Error() + + if !strings.Contains(msg, "target commit") { + t.Errorf("message must keep the 'target commit' substring: TC12 matches on it and it is how "+ + "this failure is found across a fleet. Got: %s", msg) + } +}