diff --git a/go.mod b/go.mod index 210f5c6..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.7.2 + 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 a4723e7..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.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.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/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..cc75cc8 100644 --- a/internal/controllers/localresource/localresource.go +++ b/internal/controllers/localresource/localresource.go @@ -163,6 +163,11 @@ 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, 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) { cr, ok := mg.(*localResourcev1alpha1.LocalResource) if !ok { @@ -175,6 +180,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 +193,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. 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, ResourceUpToDate: true, @@ -228,6 +239,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 +259,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..0d17d72 --- /dev/null +++ b/internal/controllers/repo/observe_ready_test.go @@ -0,0 +1,193 @@ +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) +} + +// 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) + + 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 a07e458..f7d3575 100644 --- a/internal/controllers/repo/repo.go +++ b/internal/controllers/repo/repo.go @@ -121,6 +121,11 @@ 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, 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) { cr, ok := mg.(*repov1alpha1.Repo) if !ok { @@ -128,6 +133,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 +146,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. 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, ResourceUpToDate: true, @@ -156,6 +167,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 +221,25 @@ 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( + // + // 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. + // + // 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. + 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 +249,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,