From 2c5ad9626e42610c12aa45c803db9a9628206f76 Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Tue, 22 Sep 2026 17:42:33 +0200 Subject: [PATCH] fix(repo): say why a target commit became unreachable instead of implying a bad pin (#23) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine Repo CRs began failing within 41 seconds of each other with: target commit not found on branch main while enableUpdate is false That message describes what the check found, and reads as though each resource had been pinned to a commit that was never there. Nothing was pinned wrong. Their shared target repository was recreated at 12:24:15Z — its entire main history begins one second later — so the recorded commits were discarded along with the history containing them. Nine CRs, two distinct SHAs, one repository, one event. The message now names the cause and BOTH remedies. Offering only enableUpdate would push operators toward allowing overwrites they may not want; re-creating the resource re-pins to the current history instead. Deliberately still an error. My first attempt converted this into a quiet Ready=False with ResourceUpToDate: true, reasoning that the branch below already logs-and-continues for the analogous origin-commit case. The integration suite rejected that, correctly: TC12-DisableUpdateDetectsTargetDrift recreates the target repository and asserts Synced=False/ReconcileError, and TC13-AutoHealAfterEnableUpdateFalse covers the heal once the flag is flipped. Together they pin a designed contract — enableUpdate: false surfaces an error until a human chooses — and a real decision is owed here, so reporting it until it is made is right. This is NOT the #22 situation despite both being reached from Observe. There the provider refused on an ambiguity nothing would ever resolve, stranding the resource with no path back. Here a person has a genuine choice to make. Same function, two exits, two different defects — only one of them is a deadlock. The "target commit" substring is preserved: TC12 matches on it, and it is how this failure is found across a fleet. Nine resources failing for one reason should be greppable as one string. Co-Authored-By: Claude Opus 5 (1M context) --- internal/controllers/repo/repo.go | 19 +++++- .../controllers/repo/unreachable_commit.go | 31 ++++++++++ .../repo/unreachable_commit_test.go | 58 +++++++++++++++++++ 3 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 internal/controllers/repo/unreachable_commit.go create mode 100644 internal/controllers/repo/unreachable_commit_test.go 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) + } +}