From beb965f95aa539b403391b699f10d6da1a44b7f3 Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Wed, 4 Nov 2026 08:44:37 +0100 Subject: [PATCH 1/3] =?UTF-8?q?feat:=20provider-runtime=20v1.8.0=20?= =?UTF-8?q?=E2=80=94=20set=20Ready=20on=20every=20successful=20Observe?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit provider-runtime v1.8.0 makes Ready the provider's to set. When Connect or Observe fails, the runtime sets Ready=Unknown/NotObserved, unless Ready is Deleting or the error is wrapped with reconciler.ReadyAssessed. Every successful Observe must then set Ready again, or NotObserved stays and the runtime emits a ReadyNotSetAfterObserve Warning. Repo Observe: - deletion after a ReconcileError: Deleting (the object is being released) - nothing pushed yet: Unavailable - pushed commit no longer on the target branch, enableUpdate=true: Unavailable, as on the enableUpdate=false branch - origin ahead, enableUpdate=true: Available; the drift is carried by ResourceUpToDate=false - the enableUpdate=false unreachable-commit error is wrapped with ReadyAssessed, so the Unavailable failSync sets survives LocalResource Observe: - deletion after a ReconcileError: Deleting - nothing committed yet: Unavailable - an older spec committed: Available, also when syncEnabled=false makes Update a no-op Tests call reconcilertest.AssertObserveSetsReady once per success path, against local git repositories (new internal/clients/git/gittest), and check the ReadyAssessed error and that a could-not-tell error stays plain. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016RAxvnK22nQGNMCWkPMj9z --- go.mod | 2 +- go.sum | 4 +- internal/clients/git/gittest/gittest.go | 66 +++++++ .../localresource/localresource.go | 18 ++ .../localresource/observe_ready_test.go | 138 ++++++++++++++ .../controllers/repo/observe_ready_test.go | 173 ++++++++++++++++++ internal/controllers/repo/repo.go | 30 ++- 7 files changed, 426 insertions(+), 5 deletions(-) create mode 100644 internal/clients/git/gittest/gittest.go create mode 100644 internal/controllers/localresource/observe_ready_test.go create mode 100644 internal/controllers/repo/observe_ready_test.go diff --git a/go.mod b/go.mod index 210f5c6..785672a 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/go-git/go-git/v5 v5.19.2 github.com/go-logr/logr v1.4.4 github.com/krateo-platformops/plumbing v1.14.3 - github.com/krateo-platformops/provider-runtime v1.7.2 + github.com/krateo-platformops/provider-runtime v1.8.0 github.com/moby/moby/api v1.52.0 github.com/moby/moby/client v0.1.0 github.com/pkg/errors v0.9.1 diff --git a/go.sum b/go.sum index a4723e7..62b6453 100644 --- a/go.sum +++ b/go.sum @@ -149,8 +149,8 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/krateo-platformops/plumbing v1.14.3 h1:g60QTgbhIsZVTFq9JZveNx2QZauVyFeWHUAVxahxKRU= github.com/krateo-platformops/plumbing v1.14.3/go.mod h1:JMHwVbS2vdA/OSm3Gi3QCDTntFjPmrGtqZ6LQuhciZQ= -github.com/krateo-platformops/provider-runtime v1.7.2 h1:1Q+NnVcJqMYmK0+OqT1uR1isYJAo8Ec4wvpzoTsuk2s= -github.com/krateo-platformops/provider-runtime v1.7.2/go.mod h1:3j80GRyb7mfg+FweV0Ev1Si/0n72lZEujb3JyYL3BMQ= +github.com/krateo-platformops/provider-runtime v1.8.0 h1:2Bq4vQ4r9BPOk6z0hV7lGpi21BpCFEZ5kbaq5UL6UEk= +github.com/krateo-platformops/provider-runtime v1.8.0/go.mod h1:3j80GRyb7mfg+FweV0Ev1Si/0n72lZEujb3JyYL3BMQ= github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc= github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/mailru/easyjson v0.9.0 h1:PrnmzHw7262yW8sTBwxi1PdJA3Iw/EKBa8psRf7d9a4= diff --git a/internal/clients/git/gittest/gittest.go b/internal/clients/git/gittest/gittest.go new file mode 100644 index 0000000..626ff46 --- /dev/null +++ b/internal/clients/git/gittest/gittest.go @@ -0,0 +1,66 @@ +// Package gittest builds throwaway git repositories on local disk for tests. +// +// The controllers' Observe reads remote history through the git client, which accepts a local +// path as a remote URL. A repository made here lets a test drive Observe down a chosen path with +// no network and no git server. go-git serves a local path by running the git-upload-pack binary, +// so git must be installed where the tests run. +package gittest + +import ( + "os" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" +) + +// Repo is a non-bare repository in a test's temporary directory. +type Repo struct { + t testing.TB + repo *git.Repository + commits int + // URL is the repository's path, usable wherever the git client takes a remote URL. + URL string +} + +// NewRepo creates an empty repository whose default branch is branch. +func NewRepo(t testing.TB, branch string) *Repo { + t.Helper() + dir := t.TempDir() + r, err := git.PlainInitWithOptions(dir, &git.PlainInitOptions{ + InitOptions: git.InitOptions{DefaultBranch: plumbing.NewBranchReferenceName(branch)}, + }) + if err != nil { + t.Fatalf("initialising test repository: %v", err) + } + return &Repo{t: t, repo: r, URL: dir} +} + +// Commit adds a commit with the given message on the current branch and returns its hash. Each +// commit changes a file, so it is never empty. +func (r *Repo) Commit(message string) string { + r.t.Helper() + wt, err := r.repo.Worktree() + if err != nil { + r.t.Fatalf("opening worktree: %v", err) + } + r.commits++ + name := "file-" + strconv.Itoa(r.commits) + ".txt" + if err := os.WriteFile(filepath.Join(r.URL, name), []byte(message), 0o600); err != nil { + r.t.Fatalf("writing %s: %v", name, err) + } + if _, err := wt.Add(name); err != nil { + r.t.Fatalf("staging %s: %v", name, err) + } + hash, err := wt.Commit(message, &git.CommitOptions{ + Author: &object.Signature{Name: "gittest", Email: "gittest@example.com", When: time.Now()}, + }) + if err != nil { + r.t.Fatalf("committing: %v", err) + } + return hash.String() +} diff --git a/internal/controllers/localresource/localresource.go b/internal/controllers/localresource/localresource.go index ddb2b95..3efd564 100644 --- a/internal/controllers/localresource/localresource.go +++ b/internal/controllers/localresource/localresource.go @@ -163,6 +163,10 @@ type external struct { rec record.EventRecorder } +// Observe follows the provider-runtime Ready contract (v1.8.0): every path that returns a nil +// error sets Ready. No error here follows a Ready write, so none is wrapped with +// reconciler.ReadyAssessed: each one means "could not tell", and the runtime answers it with +// Ready=Unknown/NotObserved. TestObserveSetsReady covers each success path. func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler.ExternalObservation, error) { cr, ok := mg.(*localResourcev1alpha1.LocalResource) if !ok { @@ -175,6 +179,8 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler log.Debug("Observing resource") + // Ready is already Deleting, which the runtime keeps whatever happens, so there is nothing + // new to report. if cr.GetCondition(commonv1.TypeReady).Reason == commonv1.ReasonDeleting { return reconciler.ExternalObservation{ ResourceExists: false, @@ -186,6 +192,10 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler if !meta.IsActionAllowed(cr, meta.ActionDelete) { log.Debug("External resource should not be deleted by provider, skip deleting.") } else { + // The LocalResource is being deleted and nothing is removed from the target, so the + // runtime releases it. Synced=ReconcileError is also what a failed Observe leaves next + // to Ready=NotObserved, so Ready must be set here: the object is going away, so Deleting. + cr.Status.SetConditions(commonv1.Deleting()) return reconciler.ExternalObservation{ ResourceExists: false, ResourceUpToDate: true, @@ -228,6 +238,11 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler return reconciler.ExternalObservation{}, err } else if hash.IsZero() && hasGitProviderPreviuslyCommitted { log.Debug("Target LocalResource spec is not up-to-date", "commitId", cr.Status.TargetCommitId, "branch", cr.Status.TargetBranch) + // An earlier commit of this LocalResource is on the branch, so the target holds an older + // version of it: Available. ResourceUpToDate=false carries the drift. Update commits the current + // spec, unless spec.syncEnabled is false or the management policy blocks updates; the + // file then stays as it is, and so does Available. + cr.Status.SetConditions(commonv1.Available()) return reconciler.ExternalObservation{ ResourceExists: true, ResourceUpToDate: false, @@ -243,6 +258,9 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler } if meta.GetExternalName(cr) == "" { + // Nothing has been committed yet, so there is nothing to use. Create, when the management + // policy allows it, moves this to Creating. + cr.Status.SetConditions(commonv1.Unavailable().WithMessage("the resource has not been committed to the target repository yet")) return reconciler.ExternalObservation{ ResourceExists: false, ResourceUpToDate: true, diff --git a/internal/controllers/localresource/observe_ready_test.go b/internal/controllers/localresource/observe_ready_test.go new file mode 100644 index 0000000..0d1d0af --- /dev/null +++ b/internal/controllers/localresource/observe_ready_test.go @@ -0,0 +1,138 @@ +package localresource + +import ( + "context" + "errors" + "testing" + + commonv1 "github.com/krateo-platformops/provider-runtime/apis/common/v1" + "github.com/krateo-platformops/provider-runtime/pkg/logging" + "github.com/krateo-platformops/provider-runtime/pkg/reconciler" + "github.com/krateo-platformops/provider-runtime/pkg/reconciler/reconcilertest" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + localresourcev1alpha1 "github.com/krateoplatformops/git-provider/apis/localresource/v1alpha1" + "github.com/krateoplatformops/git-provider/internal/clients/git/gittest" + credentialhelper "github.com/krateoplatformops/git-provider/internal/controllers/common/credentialHelper" + "github.com/krateoplatformops/git-provider/internal/controllers/common/footer" +) + +const branch = "main" + +func newObservedLocalResource(url, content string) *localresourcev1alpha1.LocalResource { + return &localresourcev1alpha1.LocalResource{ + ObjectMeta: metav1.ObjectMeta{Name: "sample", Namespace: "test-system"}, + Spec: localresourcev1alpha1.LocalResourceSpec{ + FromResource: localresourcev1alpha1.FromResource{FromString: &content}, + ToRepo: localresourcev1alpha1.LocalResourceOpts{Url: url, Branch: branch}, + }, + } +} + +// commitFor commits to target as the provider does for cr, with the footer that names cr and +// the hash of its spec. +func commitFor(t *testing.T, target *gittest.Repo, cr *localresourcev1alpha1.LocalResource) string { + t.Helper() + f, err := footer.LocalResourceCommitFooter(context.Background(), cr, nil) + require.NoError(t, err) + return target.Commit("update\n\n" + f) +} + +func newObserveExternal() *external { + return &external{log: logging.NewNopLogger(), cfg: &gitClientOpts{ToRepoCreds: &credentialhelper.Credentials{}}} +} + +// Every success path of Observe must set Ready (the provider-runtime v1.8.0 Ready contract). One +// case per path, so a new path without a case stands out in review, and a path that stops setting +// Ready fails here instead of as a ReadyNotSetAfterObserve Warning on a cluster. +func TestObserveSetsReady(t *testing.T) { + var ( + absent = reconciler.ExternalObservation{ResourceExists: false, ResourceUpToDate: true} + drifted = reconciler.ExternalObservation{ResourceExists: true, ResourceUpToDate: false} + current = reconciler.ExternalObservation{ResourceExists: true, ResourceUpToDate: true} + ) + cases := map[string]struct { + // setup prepares the target repository and the LocalResource. + setup func(t *testing.T, target *gittest.Repo, cr *localresourcev1alpha1.LocalResource) + // wantObservation pins the path the case takes; wantReady is what that path reports. + wantObservation reconciler.ExternalObservation + wantReady commonv1.ConditionReason + }{ + // The deletion shortcut. Synced=ReconcileError is what a failed Observe leaves next to + // NotObserved, so this path does run right after one. + "DeletedAfterReconcileError": { + setup: func(_ *testing.T, _ *gittest.Repo, cr *localresourcev1alpha1.LocalResource) { + cr.DeletionTimestamp = &metav1.Time{Time: metav1.Now().Time} + cr.Status.SetConditions(commonv1.ReconcileError(errors.New("observe failed"))) + }, + wantObservation: absent, + wantReady: commonv1.ReasonDeleting, + }, + "NotCommittedYet": { + setup: func(_ *testing.T, target *gittest.Repo, _ *localresourcev1alpha1.LocalResource) { + target.Commit("someone else's commit") + }, + wantObservation: absent, + wantReady: commonv1.ReasonUnavailable, + }, + // The branch holds a commit for an earlier spec of this LocalResource. Update would + // commit the current one, but it does nothing while syncEnabled is false; Ready must + // still be set. + "DriftedWithSyncDisabled": { + setup: func(t *testing.T, target *gittest.Repo, cr *localresourcev1alpha1.LocalResource) { + earlier := cr.DeepCopy() + older := "kind: ConfigMap # earlier" + earlier.Spec.FromResource.FromString = &older + cr.Status.TargetCommitId = commitFor(t, target, earlier) + cr.Spec.SyncEnabled = false + }, + wantObservation: drifted, + wantReady: commonv1.ReasonAvailable, + }, + "UpToDate": { + setup: func(t *testing.T, target *gittest.Repo, cr *localresourcev1alpha1.LocalResource) { + cr.Status.TargetCommitId = commitFor(t, target, cr) + }, + wantObservation: current, + wantReady: commonv1.ReasonAvailable, + }, + // Create found the target already holding the content, so it recorded the branch head + // without making a commit, and there is no footer to find. The recorded commit is what + // the resource is. + "RecordedWithoutFooter": { + setup: func(_ *testing.T, target *gittest.Repo, cr *localresourcev1alpha1.LocalResource) { + cr.Status.TargetCommitId = target.Commit("content already there") + }, + wantObservation: current, + wantReady: commonv1.ReasonAvailable, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + target := gittest.NewRepo(t, branch) + cr := newObservedLocalResource(target.URL, "kind: ConfigMap") + tc.setup(t, target, cr) + + obs, err := newObserveExternal().Observe(context.Background(), cr.DeepCopy()) + require.NoError(t, err) + require.Equal(t, tc.wantObservation, obs, "the case does not take the path it names") + + reconcilertest.AssertObserveSetsReady(t, context.Background(), newObserveExternal(), cr) + require.Equal(t, tc.wantReady, cr.GetCondition(commonv1.TypeReady).Reason) + }) + } +} + +// An Observe that could not read the target has not assessed Ready, so its error must stay plain: +// the runtime then reports Ready=NotObserved, which is the truthful answer. +func TestObserveUnreadableTargetIsNotReadyAssessed(t *testing.T) { + target := gittest.NewRepo(t, branch) + cr := newObservedLocalResource(target.URL+"-does-not-exist", "kind: ConfigMap") + + _, err := newObserveExternal().Observe(context.Background(), cr) + + require.Error(t, err) + require.False(t, reconciler.IsReadyAssessed(err), "an Observe that could not tell must not claim it set Ready: %v", err) +} diff --git a/internal/controllers/repo/observe_ready_test.go b/internal/controllers/repo/observe_ready_test.go new file mode 100644 index 0000000..950abdf --- /dev/null +++ b/internal/controllers/repo/observe_ready_test.go @@ -0,0 +1,173 @@ +package repo + +import ( + "context" + "errors" + "strings" + "testing" + + commonv1 "github.com/krateo-platformops/provider-runtime/apis/common/v1" + "github.com/krateo-platformops/provider-runtime/pkg/logging" + "github.com/krateo-platformops/provider-runtime/pkg/reconciler" + "github.com/krateo-platformops/provider-runtime/pkg/reconciler/reconcilertest" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + repov1alpha1 "github.com/krateoplatformops/git-provider/apis/repo/v1alpha1" + "github.com/krateoplatformops/git-provider/internal/clients/git/gittest" +) + +const branch = "main" + +// unreachableCommit is a well-formed commit id that no fixture repository contains. +var unreachableCommit = strings.Repeat("0", 39) + "1" + +// observeFixture is an origin and a target repository on local disk, each with one commit, and +// a Repo that last synced them: OriginCommitId and TargetCommitId are those commits. +type observeFixture struct { + origin, target *gittest.Repo + originCommit string + targetCommit string +} + +func newObserveFixture(t *testing.T) observeFixture { + t.Helper() + f := observeFixture{origin: gittest.NewRepo(t, branch), target: gittest.NewRepo(t, branch)} + f.originCommit = f.origin.Commit("origin content") + f.targetCommit = f.target.Commit("first commit") + return f +} + +func (f observeFixture) syncedRepo() *repov1alpha1.Repo { + cr := &repov1alpha1.Repo{ + ObjectMeta: metav1.ObjectMeta{Name: "sample", Namespace: "test-system"}, + Spec: repov1alpha1.RepoSpec{ + FromRepo: repov1alpha1.FromRepoOpts{RepoOpts: repov1alpha1.RepoOpts{Url: f.origin.URL, Branch: branch}}, + ToRepo: repov1alpha1.RepoOpts{Url: f.target.URL, Branch: branch}, + }, + } + cr.Status.OriginCommitId = f.originCommit + cr.Status.TargetCommitId = f.targetCommit + cr.Status.OriginBranch = branch + cr.Status.TargetBranch = branch + return cr +} + +func newObserveExternal() *external { + return &external{log: logging.NewNopLogger(), cfg: &externalClientOpts{}} +} + +// Every success path of Observe must set Ready (the provider-runtime v1.8.0 Ready contract). One +// case per path, so a new path without a case stands out in review, and a path that stops setting +// Ready fails here instead of as a ReadyNotSetAfterObserve Warning on a cluster. +func TestObserveSetsReady(t *testing.T) { + var ( + absent = reconciler.ExternalObservation{ResourceExists: false, ResourceUpToDate: true} + drifted = reconciler.ExternalObservation{ResourceExists: true, ResourceUpToDate: false} + current = reconciler.ExternalObservation{ResourceExists: true, ResourceUpToDate: true} + ) + cases := map[string]struct { + mutate func(f observeFixture, cr *repov1alpha1.Repo) + // wantObservation pins the path the case takes; wantReady is what that path reports. + wantObservation reconciler.ExternalObservation + wantReady commonv1.ConditionReason + }{ + // The deletion shortcut. Synced=ReconcileError is what a failed Observe leaves next to + // NotObserved, so this path does run right after one. + "DeletedAfterReconcileError": { + mutate: func(_ observeFixture, cr *repov1alpha1.Repo) { + cr.DeletionTimestamp = &metav1.Time{Time: metav1.Now().Time} + cr.Status.SetConditions(commonv1.ReconcileError(errors.New("observe failed"))) + }, + wantObservation: absent, + wantReady: commonv1.ReasonDeleting, + }, + "NotSyncedYet": { + mutate: func(_ observeFixture, cr *repov1alpha1.Repo) { + cr.Status = repov1alpha1.RepoStatus{} + }, + wantObservation: absent, + wantReady: commonv1.ReasonUnavailable, + }, + // The target history no longer holds the pushed commit, and enableUpdate lets Update + // push again. + "TargetCommitUnreachableWithUpdates": { + mutate: func(_ observeFixture, cr *repov1alpha1.Repo) { + cr.Spec.EnableUpdate = true + cr.Status.TargetCommitId = unreachableCommit + }, + wantObservation: drifted, + wantReady: commonv1.ReasonUnavailable, + }, + "OriginAheadWithUpdates": { + mutate: func(f observeFixture, cr *repov1alpha1.Repo) { + cr.Spec.EnableUpdate = true + f.origin.Commit("newer origin content") + }, + wantObservation: drifted, + wantReady: commonv1.ReasonAvailable, + }, + "OriginAheadWithoutUpdates": { + mutate: func(f observeFixture, _ *repov1alpha1.Repo) { + f.origin.Commit("newer origin content") + }, + wantObservation: current, + wantReady: commonv1.ReasonAvailable, + }, + "UpToDate": { + mutate: func(observeFixture, *repov1alpha1.Repo) {}, + wantObservation: current, + wantReady: commonv1.ReasonAvailable, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + f := newObserveFixture(t) + cr := f.syncedRepo() + tc.mutate(f, cr) + + obs, err := newObserveExternal().Observe(context.Background(), cr.DeepCopy()) + require.NoError(t, err) + require.Equal(t, tc.wantObservation, obs, "the case does not take the path it names") + + reconcilertest.AssertObserveSetsReady(t, context.Background(), newObserveExternal(), cr) + require.Equal(t, tc.wantReady, cr.GetCondition(commonv1.TypeReady).Reason) + }) + } +} + +// With enableUpdate=false, an unreachable target commit is an error for a person to resolve +// (#23). Observe has read the target by then and set Ready=Unavailable through failSync, so the +// error is ReadyAssessed and the runtime must keep Unavailable rather than write NotObserved. +func TestObserveUnreachableTargetCommitIsReadyAssessed(t *testing.T) { + f := newObserveFixture(t) + cr := f.syncedRepo() + cr.Status.TargetCommitId = unreachableCommit + cr.Status.SetConditions(commonv1.NotObserved()) + + _, err := newObserveExternal().Observe(context.Background(), cr) + + require.Error(t, err) + require.True(t, reconciler.IsReadyAssessed(err), "Observe set Ready before failing, so the error must be ReadyAssessed: %v", err) + require.Equal(t, unreachableTargetCommitError(cr.Status.TargetCommitId, branch, f.target.URL).Error(), err.Error(), + "ReadyAssessed must not change the message") + + ready := cr.GetCondition(commonv1.TypeReady) + require.Equal(t, commonv1.ReasonUnavailable, ready.Reason) + require.Equal(t, metav1.ConditionFalse, ready.Status) + require.Equal(t, commonv1.ReasonReconcileError, cr.GetCondition(commonv1.TypeSynced).Reason) +} + +// An Observe that could not read the remote has not assessed Ready, so its error must stay plain: +// the runtime then reports Ready=NotObserved, which is the truthful answer. +func TestObserveUnreadableRemoteIsNotReadyAssessed(t *testing.T) { + f := newObserveFixture(t) + cr := f.syncedRepo() + cr.Spec.FromRepo.Url = f.origin.URL + "-does-not-exist" + + _, err := newObserveExternal().Observe(context.Background(), cr) + + require.Error(t, err) + require.False(t, reconciler.IsReadyAssessed(err), "an Observe that could not tell must not claim it set Ready: %v", err) +} diff --git a/internal/controllers/repo/repo.go b/internal/controllers/repo/repo.go index a07e458..b8a985b 100644 --- a/internal/controllers/repo/repo.go +++ b/internal/controllers/repo/repo.go @@ -121,6 +121,10 @@ type external struct { rec plumbingevent.APIRecorder } +// Observe follows the provider-runtime Ready contract (v1.8.0): every path that returns a nil +// error sets Ready, and an error returned after Ready was set is wrapped with +// reconciler.ReadyAssessed. A plain error means "could not tell", and the runtime answers it with +// Ready=Unknown/NotObserved. TestObserveSetsReady covers each success path. func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler.ExternalObservation, error) { cr, ok := mg.(*repov1alpha1.Repo) if !ok { @@ -128,6 +132,8 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler } e.log.Info("Observing resource") + // Ready is already Deleting, which the runtime keeps whatever happens, so there is nothing + // new to report. if cr.GetCondition(commonv1.TypeReady).Reason == commonv1.ReasonDeleting { return reconciler.ExternalObservation{ ResourceExists: false, @@ -139,6 +145,10 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler if !meta.IsActionAllowed(cr, meta.ActionDelete) { e.log.Debug("External resource should not be deleted by provider, skip deleting.") } else { + // The Repo is being deleted and nothing is removed from the target, so the runtime + // releases it. Synced=ReconcileError is also what a failed Observe leaves next to + // Ready=NotObserved, so Ready must be set here: the object is going away, so Deleting. + cr.Status.SetConditions(commonv1.Deleting()) return reconciler.ExternalObservation{ ResourceExists: false, ResourceUpToDate: true, @@ -156,6 +166,9 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler } if meta.GetExternalName(cr) == "" { + // Nothing has been pushed yet, so there is nothing to use. Create, when the management + // policy allows it, moves this to Creating. + cr.Status.SetConditions(commonv1.Unavailable().WithMessage("the target repository has not been synced yet")) return reconciler.ExternalObservation{ ResourceExists: false, ResourceUpToDate: true, @@ -207,9 +220,18 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler // 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)) + // + // failSync sets Ready=Unavailable. That is an assessment, not a failure to observe: the + // target was read and the pushed commit is not on it. ReadyAssessed keeps it, instead + // of the runtime replacing it with NotObserved. + return reconciler.ExternalObservation{}, reconciler.ReadyAssessed(e.failSync(ctx, cr, unreachableTargetCommitError( + cr.Status.TargetCommitId, cr.Status.TargetBranch, cr.Spec.ToRepo.Url))) } + // Same finding as above, but enableUpdate lets Update push again. Until it does, the + // target no longer holds what this Repo pushed: Unavailable, as on the branch above. + cr.Status.SetConditions(commonv1.Unavailable().WithMessage(fmt.Sprintf( + "target commit %s is no longer reachable from branch %s of %s", + cr.Status.TargetCommitId, cr.Status.TargetBranch, cr.Spec.ToRepo.Url))) return reconciler.ExternalObservation{ ResourceExists: true, ResourceUpToDate: false, @@ -219,6 +241,10 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler if ptr.Deref(latestCommit, "") != cr.Status.OriginCommitId { e.log.Debug("Origin commit is no longer the latest commit on origin repository", "commitId", cr.Status.OriginCommitId, "branch", cr.Status.OriginBranch) if cr.Spec.EnableUpdate { + // The target still holds the last sync, so it is usable: Available, as on the + // enableUpdate=false branch below. ResourceUpToDate=false carries the drift, and + // Update pushes the newer origin commits. + cr.Status.SetConditions(commonv1.Available()) return reconciler.ExternalObservation{ ResourceExists: true, ResourceUpToDate: false, From 4bb542fdd064e3766c6204e8cbead9857da0e702 Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Wed, 4 Nov 2026 09:41:05 +0100 Subject: [PATCH 2/3] feat: provider-runtime v1.8.1; keep Ready=Deleting when Observe errors during deletion v1.8.1 stops requiring Ready from a successful Observe on a resource being deleted, because the runtime's deletion branch sets Deleting itself. The Deleting writes on the deletion returns stay: they are allowed and keep every path taken while deleting on the same Ready. That exemption covers successes only. On an Observe error the runtime writes NotObserved unless Deleting is already on the object. So Observe now wraps the old body (observe) and sets Deleting before returning an error for a resource being deleted. This includes the unreachable-commit error, where failSync had set Unavailable. Tests cover both controllers. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016RAxvnK22nQGNMCWkPMj9z --- go.mod | 2 +- go.sum | 4 +-- .../localresource/localresource.go | 19 ++++++++++-- .../localresource/observe_ready_test.go | 17 +++++++++++ .../controllers/repo/observe_ready_test.go | 30 +++++++++++++++++++ internal/controllers/repo/repo.go | 17 +++++++++-- 6 files changed, 81 insertions(+), 8 deletions(-) diff --git a/go.mod b/go.mod index 785672a..c616200 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/go-git/go-git/v5 v5.19.2 github.com/go-logr/logr v1.4.4 github.com/krateo-platformops/plumbing v1.14.3 - github.com/krateo-platformops/provider-runtime v1.8.0 + github.com/krateo-platformops/provider-runtime v1.8.1 github.com/moby/moby/api v1.52.0 github.com/moby/moby/client v0.1.0 github.com/pkg/errors v0.9.1 diff --git a/go.sum b/go.sum index 62b6453..b6bc395 100644 --- a/go.sum +++ b/go.sum @@ -149,8 +149,8 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/krateo-platformops/plumbing v1.14.3 h1:g60QTgbhIsZVTFq9JZveNx2QZauVyFeWHUAVxahxKRU= github.com/krateo-platformops/plumbing v1.14.3/go.mod h1:JMHwVbS2vdA/OSm3Gi3QCDTntFjPmrGtqZ6LQuhciZQ= -github.com/krateo-platformops/provider-runtime v1.8.0 h1:2Bq4vQ4r9BPOk6z0hV7lGpi21BpCFEZ5kbaq5UL6UEk= -github.com/krateo-platformops/provider-runtime v1.8.0/go.mod h1:3j80GRyb7mfg+FweV0Ev1Si/0n72lZEujb3JyYL3BMQ= +github.com/krateo-platformops/provider-runtime v1.8.1 h1:JIHUIFN2nGq46hRKUdMK5SIvvv81WBZ9iSLVmE1co1g= +github.com/krateo-platformops/provider-runtime v1.8.1/go.mod h1:3j80GRyb7mfg+FweV0Ev1Si/0n72lZEujb3JyYL3BMQ= github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc= github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/mailru/easyjson v0.9.0 h1:PrnmzHw7262yW8sTBwxi1PdJA3Iw/EKBa8psRf7d9a4= diff --git a/internal/controllers/localresource/localresource.go b/internal/controllers/localresource/localresource.go index 3efd564..347a747 100644 --- a/internal/controllers/localresource/localresource.go +++ b/internal/controllers/localresource/localresource.go @@ -167,7 +167,20 @@ type external struct { // error sets Ready. No error here follows a Ready write, so none is wrapped with // reconciler.ReadyAssessed: each one means "could not tell", and the runtime answers it with // Ready=Unknown/NotObserved. TestObserveSetsReady covers each success path. +// +// A resource being deleted is the exception. Ready=Deleting holds whatever the target answers, +// and the runtime keeps a Deleting it finds on the object instead of writing NotObserved. So an +// error returned while the resource is being deleted sets Deleting first. A successful Observe +// owes nothing there, because the runtime's deletion branch sets Deleting itself (v1.8.1). func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler.ExternalObservation, error) { + obs, err := e.observe(ctx, mg) + if err != nil && meta.WasDeleted(mg) { + mg.SetConditions(commonv1.Deleting()) + } + return obs, err +} + +func (e *external) observe(ctx context.Context, mg resource.Managed) (reconciler.ExternalObservation, error) { cr, ok := mg.(*localResourcev1alpha1.LocalResource) if !ok { return reconciler.ExternalObservation{}, errors.New(errNotLocalResource) @@ -192,9 +205,9 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler if !meta.IsActionAllowed(cr, meta.ActionDelete) { log.Debug("External resource should not be deleted by provider, skip deleting.") } else { - // The LocalResource is being deleted and nothing is removed from the target, so the - // runtime releases it. Synced=ReconcileError is also what a failed Observe leaves next - // to Ready=NotObserved, so Ready must be set here: the object is going away, so Deleting. + // The LocalResource is being deleted and nothing is removed from the target, so the runtime + // releases it. Since v1.8.1 the runtime does not require Ready here; Deleting is set + // anyway so that every path taken while deleting reports the same Ready. cr.Status.SetConditions(commonv1.Deleting()) return reconciler.ExternalObservation{ ResourceExists: false, diff --git a/internal/controllers/localresource/observe_ready_test.go b/internal/controllers/localresource/observe_ready_test.go index 0d1d0af..a268710 100644 --- a/internal/controllers/localresource/observe_ready_test.go +++ b/internal/controllers/localresource/observe_ready_test.go @@ -136,3 +136,20 @@ func TestObserveUnreadableTargetIsNotReadyAssessed(t *testing.T) { require.Error(t, err) require.False(t, reconciler.IsReadyAssessed(err), "an Observe that could not tell must not claim it set Ready: %v", err) } + +// While the LocalResource is being deleted, Ready=Deleting holds whatever the target answers. The +// runtime keeps a Deleting it finds on the object, but writes NotObserved over anything else, so +// an Observe error on a LocalResource being deleted must set Deleting first. Synced is not +// ReconcileError here, so Observe reads the target and fails instead of taking the deletion +// shortcut. +func TestObserveErrorWhileDeletingSetsDeleting(t *testing.T) { + target := gittest.NewRepo(t, branch) + cr := newObservedLocalResource(target.URL+"-does-not-exist", "kind: ConfigMap") + cr.DeletionTimestamp = &metav1.Time{Time: metav1.Now().Time} + cr.Status.SetConditions(commonv1.Available()) + + _, err := newObserveExternal().Observe(context.Background(), cr) + + require.Error(t, err) + require.Equal(t, commonv1.ReasonDeleting, cr.GetCondition(commonv1.TypeReady).Reason) +} diff --git a/internal/controllers/repo/observe_ready_test.go b/internal/controllers/repo/observe_ready_test.go index 950abdf..23771d3 100644 --- a/internal/controllers/repo/observe_ready_test.go +++ b/internal/controllers/repo/observe_ready_test.go @@ -171,3 +171,33 @@ func TestObserveUnreadableRemoteIsNotReadyAssessed(t *testing.T) { require.Error(t, err) require.False(t, reconciler.IsReadyAssessed(err), "an Observe that could not tell must not claim it set Ready: %v", err) } + +// While the Repo is being deleted, Ready=Deleting holds whatever the remote answers. The runtime +// keeps a Deleting it finds on the object, but writes NotObserved over anything else, so an +// Observe error on a Repo being deleted must set Deleting first. Synced is not ReconcileError +// here, so Observe reads the remote and fails instead of taking the deletion shortcut. +func TestObserveErrorWhileDeletingSetsDeleting(t *testing.T) { + cases := map[string]func(f observeFixture, cr *repov1alpha1.Repo){ + "UnreadableRemote": func(f observeFixture, cr *repov1alpha1.Repo) { + cr.Spec.FromRepo.Url = f.origin.URL + "-does-not-exist" + }, + // failSync sets Unavailable first; Deleting must still win. + "UnreachableTargetCommit": func(_ observeFixture, cr *repov1alpha1.Repo) { + cr.Status.TargetCommitId = unreachableCommit + }, + } + for name, mutate := range cases { + t.Run(name, func(t *testing.T) { + f := newObserveFixture(t) + cr := f.syncedRepo() + cr.DeletionTimestamp = &metav1.Time{Time: metav1.Now().Time} + cr.Status.SetConditions(commonv1.Available()) + mutate(f, cr) + + _, err := newObserveExternal().Observe(context.Background(), cr) + + require.Error(t, err) + require.Equal(t, commonv1.ReasonDeleting, cr.GetCondition(commonv1.TypeReady).Reason) + }) + } +} diff --git a/internal/controllers/repo/repo.go b/internal/controllers/repo/repo.go index b8a985b..50b6ab6 100644 --- a/internal/controllers/repo/repo.go +++ b/internal/controllers/repo/repo.go @@ -125,7 +125,20 @@ type external struct { // error sets Ready, and an error returned after Ready was set is wrapped with // reconciler.ReadyAssessed. A plain error means "could not tell", and the runtime answers it with // Ready=Unknown/NotObserved. TestObserveSetsReady covers each success path. +// +// A resource being deleted is the exception. Ready=Deleting holds whatever the target answers, +// and the runtime keeps a Deleting it finds on the object instead of writing NotObserved. So an +// error returned while the resource is being deleted sets Deleting first. A successful Observe +// owes nothing there, because the runtime's deletion branch sets Deleting itself (v1.8.1). func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler.ExternalObservation, error) { + obs, err := e.observe(ctx, mg) + if err != nil && meta.WasDeleted(mg) { + mg.SetConditions(commonv1.Deleting()) + } + return obs, err +} + +func (e *external) observe(ctx context.Context, mg resource.Managed) (reconciler.ExternalObservation, error) { cr, ok := mg.(*repov1alpha1.Repo) if !ok { return reconciler.ExternalObservation{}, errNotRepo @@ -146,8 +159,8 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler e.log.Debug("External resource should not be deleted by provider, skip deleting.") } else { // The Repo is being deleted and nothing is removed from the target, so the runtime - // releases it. Synced=ReconcileError is also what a failed Observe leaves next to - // Ready=NotObserved, so Ready must be set here: the object is going away, so Deleting. + // releases it. Since v1.8.1 the runtime does not require Ready here; Deleting is set + // anyway so that every path taken while deleting reports the same Ready. cr.Status.SetConditions(commonv1.Deleting()) return reconciler.ExternalObservation{ ResourceExists: false, From f271491b458b2d57d8d22e676413c28c4cdba2c7 Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Wed, 4 Nov 2026 10:47:19 +0100 Subject: [PATCH 3/3] feat: provider-runtime v1.8.2; drop the Observe wrapper it makes redundant v1.8.2 sets Ready=Deleting itself when Connect or Observe fails on a resource with a deletion timestamp. That makes the observe() wrapper, which set Deleting before any such error, redundant, so it is removed from both controllers along with LocalResource's test of it. One case is still the provider's: the runtime checks ReadyAssessed before deletion, so it leaves a ReadyAssessed Ready alone even on a resource being deleted. Repo's unreachable-commit error is ReadyAssessed (failSync set Unavailable), so that call site now sets Deleting itself when the Repo is being deleted. Its test is kept, narrowed to this case. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016RAxvnK22nQGNMCWkPMj9z --- go.mod | 2 +- go.sum | 4 +- .../localresource/localresource.go | 16 +------ .../localresource/observe_ready_test.go | 17 -------- .../controllers/repo/observe_ready_test.go | 42 +++++++------------ internal/controllers/repo/repo.go | 27 +++++------- 6 files changed, 32 insertions(+), 76 deletions(-) diff --git a/go.mod b/go.mod index c616200..df9c55f 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/go-git/go-git/v5 v5.19.2 github.com/go-logr/logr v1.4.4 github.com/krateo-platformops/plumbing v1.14.3 - github.com/krateo-platformops/provider-runtime v1.8.1 + github.com/krateo-platformops/provider-runtime v1.8.2 github.com/moby/moby/api v1.52.0 github.com/moby/moby/client v0.1.0 github.com/pkg/errors v0.9.1 diff --git a/go.sum b/go.sum index b6bc395..0e9e551 100644 --- a/go.sum +++ b/go.sum @@ -149,8 +149,8 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/krateo-platformops/plumbing v1.14.3 h1:g60QTgbhIsZVTFq9JZveNx2QZauVyFeWHUAVxahxKRU= github.com/krateo-platformops/plumbing v1.14.3/go.mod h1:JMHwVbS2vdA/OSm3Gi3QCDTntFjPmrGtqZ6LQuhciZQ= -github.com/krateo-platformops/provider-runtime v1.8.1 h1:JIHUIFN2nGq46hRKUdMK5SIvvv81WBZ9iSLVmE1co1g= -github.com/krateo-platformops/provider-runtime v1.8.1/go.mod h1:3j80GRyb7mfg+FweV0Ev1Si/0n72lZEujb3JyYL3BMQ= +github.com/krateo-platformops/provider-runtime v1.8.2 h1:X3OxWfdsZfxOAR6IkdVD/cobCUo04mJKaxjHhSZmOQU= +github.com/krateo-platformops/provider-runtime v1.8.2/go.mod h1:3j80GRyb7mfg+FweV0Ev1Si/0n72lZEujb3JyYL3BMQ= github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc= github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/mailru/easyjson v0.9.0 h1:PrnmzHw7262yW8sTBwxi1PdJA3Iw/EKBa8psRf7d9a4= diff --git a/internal/controllers/localresource/localresource.go b/internal/controllers/localresource/localresource.go index 347a747..cc75cc8 100644 --- a/internal/controllers/localresource/localresource.go +++ b/internal/controllers/localresource/localresource.go @@ -166,21 +166,9 @@ type external struct { // Observe follows the provider-runtime Ready contract (v1.8.0): every path that returns a nil // error sets Ready. No error here follows a Ready write, so none is wrapped with // reconciler.ReadyAssessed: each one means "could not tell", and the runtime answers it with -// Ready=Unknown/NotObserved. TestObserveSetsReady covers each success path. -// -// A resource being deleted is the exception. Ready=Deleting holds whatever the target answers, -// and the runtime keeps a Deleting it finds on the object instead of writing NotObserved. So an -// error returned while the resource is being deleted sets Deleting first. A successful Observe -// owes nothing there, because the runtime's deletion branch sets Deleting itself (v1.8.1). +// Ready=Unknown/NotObserved, or with Deleting for a resource being deleted (v1.8.2). +// TestObserveSetsReady covers each success path. func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler.ExternalObservation, error) { - obs, err := e.observe(ctx, mg) - if err != nil && meta.WasDeleted(mg) { - mg.SetConditions(commonv1.Deleting()) - } - return obs, err -} - -func (e *external) observe(ctx context.Context, mg resource.Managed) (reconciler.ExternalObservation, error) { cr, ok := mg.(*localResourcev1alpha1.LocalResource) if !ok { return reconciler.ExternalObservation{}, errors.New(errNotLocalResource) diff --git a/internal/controllers/localresource/observe_ready_test.go b/internal/controllers/localresource/observe_ready_test.go index a268710..0d1d0af 100644 --- a/internal/controllers/localresource/observe_ready_test.go +++ b/internal/controllers/localresource/observe_ready_test.go @@ -136,20 +136,3 @@ func TestObserveUnreadableTargetIsNotReadyAssessed(t *testing.T) { require.Error(t, err) require.False(t, reconciler.IsReadyAssessed(err), "an Observe that could not tell must not claim it set Ready: %v", err) } - -// While the LocalResource is being deleted, Ready=Deleting holds whatever the target answers. The -// runtime keeps a Deleting it finds on the object, but writes NotObserved over anything else, so -// an Observe error on a LocalResource being deleted must set Deleting first. Synced is not -// ReconcileError here, so Observe reads the target and fails instead of taking the deletion -// shortcut. -func TestObserveErrorWhileDeletingSetsDeleting(t *testing.T) { - target := gittest.NewRepo(t, branch) - cr := newObservedLocalResource(target.URL+"-does-not-exist", "kind: ConfigMap") - cr.DeletionTimestamp = &metav1.Time{Time: metav1.Now().Time} - cr.Status.SetConditions(commonv1.Available()) - - _, err := newObserveExternal().Observe(context.Background(), cr) - - require.Error(t, err) - require.Equal(t, commonv1.ReasonDeleting, cr.GetCondition(commonv1.TypeReady).Reason) -} diff --git a/internal/controllers/repo/observe_ready_test.go b/internal/controllers/repo/observe_ready_test.go index 23771d3..0d17d72 100644 --- a/internal/controllers/repo/observe_ready_test.go +++ b/internal/controllers/repo/observe_ready_test.go @@ -172,32 +172,22 @@ func TestObserveUnreadableRemoteIsNotReadyAssessed(t *testing.T) { require.False(t, reconciler.IsReadyAssessed(err), "an Observe that could not tell must not claim it set Ready: %v", err) } -// While the Repo is being deleted, Ready=Deleting holds whatever the remote answers. The runtime -// keeps a Deleting it finds on the object, but writes NotObserved over anything else, so an -// Observe error on a Repo being deleted must set Deleting first. Synced is not ReconcileError -// here, so Observe reads the remote and fails instead of taking the deletion shortcut. -func TestObserveErrorWhileDeletingSetsDeleting(t *testing.T) { - cases := map[string]func(f observeFixture, cr *repov1alpha1.Repo){ - "UnreadableRemote": func(f observeFixture, cr *repov1alpha1.Repo) { - cr.Spec.FromRepo.Url = f.origin.URL + "-does-not-exist" - }, - // failSync sets Unavailable first; Deleting must still win. - "UnreachableTargetCommit": func(_ observeFixture, cr *repov1alpha1.Repo) { - cr.Status.TargetCommitId = unreachableCommit - }, - } - for name, mutate := range cases { - t.Run(name, func(t *testing.T) { - f := newObserveFixture(t) - cr := f.syncedRepo() - cr.DeletionTimestamp = &metav1.Time{Time: metav1.Now().Time} - cr.Status.SetConditions(commonv1.Available()) - mutate(f, cr) +// The unreachable-commit error is ReadyAssessed, and the runtime leaves a ReadyAssessed Ready +// alone even on a Repo being deleted. So Observe must set Deleting itself there, over the +// Unavailable failSync writes. (A plain error on a Repo being deleted is the runtime's to answer +// with Deleting, since v1.8.2.) Synced is not ReconcileError here, so Observe reads the target +// instead of taking the deletion shortcut. +func TestObserveUnreachableTargetCommitWhileDeletingSetsDeleting(t *testing.T) { + f := newObserveFixture(t) + cr := f.syncedRepo() + cr.DeletionTimestamp = &metav1.Time{Time: metav1.Now().Time} + cr.Status.SetConditions(commonv1.Available()) + cr.Status.TargetCommitId = unreachableCommit - _, err := newObserveExternal().Observe(context.Background(), cr) + _, err := newObserveExternal().Observe(context.Background(), cr) - require.Error(t, err) - require.Equal(t, commonv1.ReasonDeleting, cr.GetCondition(commonv1.TypeReady).Reason) - }) - } + require.Error(t, err) + require.True(t, reconciler.IsReadyAssessed(err), "Observe set Ready before failing, so the error must be ReadyAssessed: %v", err) + require.Equal(t, commonv1.ReasonDeleting, cr.GetCondition(commonv1.TypeReady).Reason) + require.Equal(t, commonv1.ReasonReconcileError, cr.GetCondition(commonv1.TypeSynced).Reason) } diff --git a/internal/controllers/repo/repo.go b/internal/controllers/repo/repo.go index 50b6ab6..f7d3575 100644 --- a/internal/controllers/repo/repo.go +++ b/internal/controllers/repo/repo.go @@ -124,21 +124,9 @@ type external struct { // Observe follows the provider-runtime Ready contract (v1.8.0): every path that returns a nil // error sets Ready, and an error returned after Ready was set is wrapped with // reconciler.ReadyAssessed. A plain error means "could not tell", and the runtime answers it with -// Ready=Unknown/NotObserved. TestObserveSetsReady covers each success path. -// -// A resource being deleted is the exception. Ready=Deleting holds whatever the target answers, -// and the runtime keeps a Deleting it finds on the object instead of writing NotObserved. So an -// error returned while the resource is being deleted sets Deleting first. A successful Observe -// owes nothing there, because the runtime's deletion branch sets Deleting itself (v1.8.1). +// Ready=Unknown/NotObserved, or with Deleting for a resource being deleted (v1.8.2). +// TestObserveSetsReady covers each success path. func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler.ExternalObservation, error) { - obs, err := e.observe(ctx, mg) - if err != nil && meta.WasDeleted(mg) { - mg.SetConditions(commonv1.Deleting()) - } - return obs, err -} - -func (e *external) observe(ctx context.Context, mg resource.Managed) (reconciler.ExternalObservation, error) { cr, ok := mg.(*repov1alpha1.Repo) if !ok { return reconciler.ExternalObservation{}, errNotRepo @@ -237,8 +225,15 @@ func (e *external) observe(ctx context.Context, mg resource.Managed) (reconciler // failSync sets Ready=Unavailable. That is an assessment, not a failure to observe: the // target was read and the pushed commit is not on it. ReadyAssessed keeps it, instead // of the runtime replacing it with NotObserved. - return reconciler.ExternalObservation{}, reconciler.ReadyAssessed(e.failSync(ctx, cr, unreachableTargetCommitError( - cr.Status.TargetCommitId, cr.Status.TargetBranch, cr.Spec.ToRepo.Url))) + // + // The runtime also leaves a ReadyAssessed Ready alone on a Repo being deleted, so + // Deleting, which holds whatever the target answers, is set here for that case. + err := e.failSync(ctx, cr, unreachableTargetCommitError( + cr.Status.TargetCommitId, cr.Status.TargetBranch, cr.Spec.ToRepo.Url)) + if meta.WasDeleted(cr) { + cr.Status.SetConditions(commonv1.Deleting()) + } + return reconciler.ExternalObservation{}, reconciler.ReadyAssessed(err) } // Same finding as above, but enableUpdate lets Update push again. Until it does, the // target no longer holds what this Repo pushed: Unavailable, as on the branch above.