feat: provider-runtime v1.8.2 — set Ready on every successful Observe

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

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=

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()
+}

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,

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)
+}