From 887b5309bfc50f9ee18cea5bba6ce9dd64e65ec9 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 2 Nov 2022 22:39:45 +0530 Subject: [PATCH] Add progressive status in gitrepository reconciler Signed-off-by: Sunny --- controllers/gitrepository_controller.go | 90 +++++-- controllers/gitrepository_controller_test.go | 232 ++++++++++++++----- 2 files changed, 248 insertions(+), 74 deletions(-) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 997f4022..40894ad1 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -50,6 +50,7 @@ import ( helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" + rreconcile "github.com/fluxcd/pkg/runtime/reconcile" "github.com/fluxcd/pkg/sourceignore" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" @@ -102,6 +103,15 @@ var gitRepositoryFailConditions = []string{ sourcev1.StorageOperationFailedCondition, } +// getPatchOptions composes patch options based on the given parameters. +// It is used as the options used when patching an object. +func getPatchOptions(ownedConditions []string, controllerName string) []patch.Option { + return []patch.Option{ + patch.WithOwnedConditions{Conditions: ownedConditions}, + patch.WithFieldOwner(controllerName), + } +} + // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=gitrepositories,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=gitrepositories/status,verbs=get;update;patch // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=gitrepositories/finalizers,verbs=get;create;update;patch;delete @@ -118,6 +128,8 @@ type GitRepositoryReconciler struct { requeueDependency time.Duration features map[string]bool + + patchOptions []patch.Option } type GitRepositoryReconcilerOptions struct { @@ -128,13 +140,15 @@ type GitRepositoryReconcilerOptions struct { // gitRepositoryReconcileFunc is the function type for all the // v1beta2.GitRepository (sub)reconcile functions. -type gitRepositoryReconcileFunc func(ctx context.Context, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) +type gitRepositoryReconcileFunc func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) func (r *GitRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error { return r.SetupWithManagerAndOptions(mgr, GitRepositoryReconcilerOptions{}) } func (r *GitRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts GitRepositoryReconcilerOptions) error { + r.patchOptions = getPatchOptions(gitRepositoryReadyCondition.Owned, r.ControllerName) + r.requeueDependency = opts.DependencyRequeueInterval if r.features == nil { @@ -167,10 +181,7 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques r.RecordSuspend(ctx, obj, obj.Spec.Suspend) // Initialize the patch helper with the current version of the object. - patchHelper, err := patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, err - } + serialPatcher := patch.NewSerialPatcher(obj, r.Client) // recResult stores the abstracted reconcile result. var recResult sreconcile.Result @@ -178,7 +189,7 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques // Always attempt to patch the object and status after each reconciliation // NOTE: The final runtime result and error are set in this block. defer func() { - summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper) + summarizeHelper := summarize.NewHelper(r.EventRecorder, serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(gitRepositoryReadyCondition), summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), @@ -227,19 +238,36 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques r.reconcileInclude, r.reconcileArtifact, } - recResult, retErr = r.reconcile(ctx, obj, reconcilers) + recResult, retErr = r.reconcile(ctx, serialPatcher, obj, reconcilers) return } // reconcile iterates through the gitRepositoryReconcileFunc tasks for the // object. It returns early on the first call that returns // reconcile.ResultRequeue, or produces an error. -func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.GitRepository, reconcilers []gitRepositoryReconcileFunc) (sreconcile.Result, error) { +func (r *GitRepositoryReconciler) reconcile(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.GitRepository, reconcilers []gitRepositoryReconcileFunc) (sreconcile.Result, error) { oldObj := obj.DeepCopy() - // Mark as reconciling if generation differs - if obj.Generation != obj.Status.ObservedGeneration { - conditions.MarkReconciling(obj, "NewGeneration", "reconciling new object generation (%d)", obj.Generation) + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") + + var recAtVal string + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + recAtVal = v + } + + // Persist reconciling if generation differs or reconciliation is requested. + switch { + case obj.Generation != obj.Status.ObservedGeneration: + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, + "processing object: new generation %d -> %d", obj.Status.ObservedGeneration, obj.Generation) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } + case recAtVal != obj.Status.GetLastHandledReconcileRequest(): + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } // Create temp dir for Git clone @@ -268,7 +296,7 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.G resErr error ) for _, rec := range reconcilers { - recResult, err := rec(ctx, obj, &commit, &includes, tmpDir) + recResult, err := rec(ctx, sp, obj, &commit, &includes, tmpDir) // Exit immediately on ResultRequeue. if recResult == sreconcile.ResultRequeue { return sreconcile.ResultRequeue, nil @@ -359,23 +387,32 @@ func (r *GitRepositoryReconciler) shouldNotify(oldObj, newObj *sourcev1.GitRepos // condition is added. // The hostname of any URL in the Status of the object are updated, to ensure // they match the Storage server hostname of current runtime. -func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, +func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.GitRepository, _ *git.Commit, _ *artifactSet, _ string) (sreconcile.Result, error) { // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) // Determine if the advertised artifact is still in storage + var artifactMissing bool if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + artifactMissing = true // Remove the condition as the artifact doesn't exist. conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { - conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + msg := "building artifact" + if artifactMissing { + msg += ": disappeared from storage" + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, msg) conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } return sreconcile.ResultSuccess, nil } @@ -417,7 +454,7 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, // and the local artifact are on the same revision, and no other source content // related configurations have changed since last reconciliation. If there's a // change, it short-circuits the whole reconciliation with an early return. -func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, +func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { // Remove previously failed source verification status conditions. The // failing verification should be recalculated. But an existing successful @@ -477,9 +514,15 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // Observe if the artifacts still match the previous included ones if artifacts.Diff(obj.Status.IncludedArtifacts) { message := fmt.Sprintf("included artifacts differ from last observed includes") - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", message) - conditions.MarkReconciling(obj, "IncludeChange", message) + if obj.Status.IncludedArtifacts != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } + conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) // Persist the ArtifactSet. *includes = *artifacts @@ -540,8 +583,13 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // Mark observations about the revision on the object if !obj.GetArtifact().HasRevision(commit.String()) { message := fmt.Sprintf("new upstream revision '%s'", commit.String()) - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) - conditions.MarkReconciling(obj, "NewRevision", message) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } return sreconcile.ResultSuccess, nil } @@ -558,7 +606,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // On a successful archive, the Artifact, Includes, observed ignore, recurse // submodules and observed include in the Status of the object are set, and the // symlink in the Storage is updated to its path. -func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, +func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { // Create potential new artifact with current available metadata @@ -672,7 +720,7 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, // v1beta2.IncludeUnavailableCondition from the object. // When the composed artifactSet differs from the current set in the Status of // the object, it marks the object with v1beta2.ArtifactOutdatedCondition=True. -func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, +func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.GitRepository, _ *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { for i, incl := range obj.Spec.Include { diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index c46a1a5e..7ecab241 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -38,7 +38,6 @@ import ( sshtestdata "golang.org/x/crypto/ssh/testdata" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/tools/record" "k8s.io/utils/pointer" kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" @@ -241,16 +240,16 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { assertConditions []metav1.Condition }{ { - name: "HTTP without secretRef makes ArtifactOutdated=True", + name: "HTTP without secretRef makes Reconciling=True", protocol: "http", want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), }, }, { - name: "HTTP with Basic Auth secret makes ArtifactOutdated=True", + name: "HTTP with Basic Auth secret makes Reconciling=True", protocol: "http", server: options{ username: "git", @@ -270,12 +269,12 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), }, }, { - name: "HTTPS with CAFile secret makes ArtifactOutdated=True", + name: "HTTPS with CAFile secret makes Reconciling=True", protocol: "https", server: options{ publicKey: tlsPublicKey, @@ -295,8 +294,8 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), }, }, { @@ -317,6 +316,8 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, beforeFunc: func(obj *sourcev1.GitRepository) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "invalid-ca"} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "foo") }, wantErr: true, assertConditions: []metav1.Condition{ @@ -326,10 +327,12 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { // // Trimming the expected error message for consistent results. *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "x509: "), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "foo"), }, }, { - name: "SSH with private key secret makes ArtifactOutdated=True", + name: "SSH with private key secret makes Reconciling=True", protocol: "ssh", server: options{ username: "git", @@ -348,12 +351,12 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/"), }, }, { - name: "SSH with password protected private key secret makes ArtifactOutdated=True", + name: "SSH with password protected private key secret makes Reconciling=True", protocol: "ssh", server: options{ username: "git", @@ -373,8 +376,8 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), }, }, { @@ -385,10 +388,46 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, beforeFunc: func(obj *sourcev1.GitRepository) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "non-existing"} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "foo") }, wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret '/non-existing': secrets \"non-existing\" not found"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "foo"), + }, + }, + { + name: "Existing artifact makes ArtifactOutdated=True", + protocol: "http", + server: options{ + username: "git", + password: "1234", + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "basic-auth", + }, + Data: map[string][]byte{ + "username": []byte("git"), + "password": []byte("1234"), + }, + }, + beforeFunc: func(obj *sourcev1.GitRepository) { + obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth"} + obj.Status = sourcev1.GitRepositoryStatus{ + Artifact: &sourcev1.Artifact{ + Revision: "staging/some-revision", + Path: randStringRunes(10), + }, + } + }, + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new upstream revision 'master/'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master/'"), }, }, } @@ -400,6 +439,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { obj := &sourcev1.GitRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "auth-strategy-", + Generation: 1, }, Spec: sourcev1.GitRepositorySpec{ Interval: metav1.Duration{Duration: interval}, @@ -468,6 +508,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { features: map[string]bool{ features.OptimizedGitClones: true, }, + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } tmpDir := t.TempDir() @@ -479,14 +520,24 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", obj.Spec.URL) } + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + var commit git.Commit var includes artifactSet + sp := patch.NewSerialPatcher(obj, r.Client) - got, err := r.reconcileSource(context.TODO(), obj, &commit, &includes, tmpDir) + got, err := r.reconcileSource(context.TODO(), sp, obj, &commit, &includes, tmpDir) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) g.Expect(commit).ToNot(BeNil()) + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -505,30 +556,31 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) wantErr bool wantRevision string wantArtifactOutdated bool + wantReconciling bool }{ { - name: "Nil reference (default branch)", - want: sreconcile.ResultSuccess, - wantRevision: "master/", - wantArtifactOutdated: true, + name: "Nil reference (default branch)", + want: sreconcile.ResultSuccess, + wantRevision: "master/", + wantReconciling: true, }, { name: "Branch", reference: &sourcev1.GitRepositoryRef{ Branch: "staging", }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", - wantArtifactOutdated: true, + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantReconciling: true, }, { name: "Tag", reference: &sourcev1.GitRepositoryRef{ Tag: "v0.1.0", }, - want: sreconcile.ResultSuccess, - wantRevision: "v0.1.0/", - wantArtifactOutdated: true, + want: sreconcile.ResultSuccess, + wantRevision: "v0.1.0/", + wantReconciling: true, }, { name: "Branch commit", @@ -536,36 +588,56 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) Branch: "staging", Commit: "", }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", - wantArtifactOutdated: true, + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantReconciling: true, }, { name: "SemVer", reference: &sourcev1.GitRepositoryRef{ SemVer: "*", }, - want: sreconcile.ResultSuccess, - wantRevision: "v2.0.0/", - wantArtifactOutdated: true, + want: sreconcile.ResultSuccess, + wantRevision: "v2.0.0/", + wantReconciling: true, }, { name: "SemVer range", reference: &sourcev1.GitRepositoryRef{ SemVer: "", - wantArtifactOutdated: true, + want: sreconcile.ResultSuccess, + wantRevision: "0.2.0/", + wantReconciling: true, }, { name: "SemVer prerelease", reference: &sourcev1.GitRepositoryRef{ SemVer: ">=1.0.0-0 <1.1.0-0", }, - wantRevision: "v1.0.0-alpha/", + wantRevision: "v1.0.0-alpha/", + want: sreconcile.ResultSuccess, + wantReconciling: true, + }, + { + name: "Existing artifact makes ArtifactOutdated=True", + reference: &sourcev1.GitRepositoryRef{ + Branch: "staging", + }, + beforeFunc: func(obj *sourcev1.GitRepository, latestRev string) { + obj.Status = sourcev1.GitRepositoryStatus{ + Artifact: &sourcev1.Artifact{ + Revision: "staging/some-revision", + Path: randStringRunes(10), + }, + } + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo") + }, want: sreconcile.ResultSuccess, + wantRevision: "staging/", wantArtifactOutdated: true, + wantReconciling: true, }, { name: "Optimized clone", @@ -581,11 +653,12 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) }, } conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultEmpty, - wantErr: true, - wantRevision: "staging/", - wantArtifactOutdated: false, + want: sreconcile.ResultEmpty, + wantErr: true, + wantRevision: "staging/", + wantReconciling: false, }, { name: "Optimized clone different ignore", @@ -603,10 +676,11 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) }, } conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", - wantArtifactOutdated: false, + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantReconciling: false, }, } @@ -632,12 +706,13 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) } r := &GitRepositoryReconciler{ - Client: fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(), + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, features: map[string]bool{ features.OptimizedGitClones: true, }, + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -645,6 +720,7 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) obj := &sourcev1.GitRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "checkout-strategy-", + Generation: 1, }, Spec: sourcev1.GitRepositorySpec{ Interval: metav1.Duration{Duration: interval}, @@ -664,9 +740,15 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) tt.beforeFunc(obj, headRef.Hash().String()) } + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + var commit git.Commit var includes artifactSet - got, err := r.reconcileSource(ctx, obj, &commit, &includes, tmpDir) + sp := patch.NewSerialPatcher(obj, r.Client) + got, err := r.reconcileSource(ctx, sp, obj, &commit, &includes, tmpDir) if err != nil { println(err.Error()) } @@ -676,7 +758,11 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) revision := strings.ReplaceAll(tt.wantRevision, "", headRef.Hash().String()) g.Expect(commit.String()).To(Equal(revision)) g.Expect(conditions.IsTrue(obj, sourcev1.ArtifactOutdatedCondition)).To(Equal(tt.wantArtifactOutdated)) + g.Expect(conditions.IsTrue(obj, meta.ReconcilingCondition)).To(Equal(tt.wantReconciling)) } + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -855,6 +941,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ @@ -873,8 +960,9 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { Hash: []byte("revision"), Reference: "refs/heads/main", } + sp := patch.NewSerialPatcher(obj, r.Client) - got, err := r.reconcileArtifact(ctx, obj, &commit, &tt.includes, tt.dir) + got, err := r.reconcileArtifact(ctx, sp, obj, &commit, &tt.includes, tt.dir) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -1000,6 +1088,7 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) { Storage: storage, requeueDependency: dependencyInterval, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ @@ -1036,7 +1125,9 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) includes = *artifactSet - got, err := r.reconcileInclude(ctx, obj, &commit, &includes, tmpDir) + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileInclude(ctx, sp, obj, &commit, &includes, tmpDir) g.Expect(obj.GetConditions()).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) if err == nil { @@ -1093,6 +1184,7 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { } } testStorage.SetArtifactURL(obj.Status.Artifact) + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, assertArtifact: &sourcev1.Artifact{ @@ -1109,6 +1201,17 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/a.txt", }, want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, + }, + { + name: "build artifact first time", + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact"), + }, }, { name: "notices missing artifact in storage", @@ -1125,7 +1228,8 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/invalid.txt", }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, { @@ -1143,6 +1247,7 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, want: sreconcile.ResultSuccess, @@ -1156,6 +1261,9 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { URL: testStorage.Hostname + "/reconcile-storage/hostname.txt", Size: int64p(int64(len("file"))), }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, }, } for _, tt := range tests { @@ -1167,23 +1275,32 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { }() r := &GitRepositoryReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", + Generation: 1, }, } if tt.beforeFunc != nil { g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed()) } + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + var c *git.Commit var as artifactSet - got, err := r.reconcileStorage(context.TODO(), obj, c, &as, "") + sp := patch.NewSerialPatcher(obj, r.Client) + got, err := r.reconcileStorage(context.TODO(), sp, obj, c, &as, "") g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -1201,6 +1318,10 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { } g.Expect(absoluteP).NotTo(BeAnExistingFile()) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -1212,6 +1333,7 @@ func TestGitRepositoryReconciler_reconcileDelete(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ @@ -1350,6 +1472,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Client: builder.Build(), features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{ @@ -1492,6 +1615,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } key := client.ObjectKeyFromObject(obj) @@ -1710,8 +1834,7 @@ func TestGitRepositoryReconciler_statusConditions(t *testing.T) { clientBuilder.WithObjects(obj) c := clientBuilder.Build() - patchHelper, err := patch.NewHelper(obj, c) - g.Expect(err).ToNot(HaveOccurred()) + serialPatcher := patch.NewSerialPatcher(obj, c) if tt.beforeFunc != nil { tt.beforeFunc(obj) @@ -1721,9 +1844,10 @@ func TestGitRepositoryReconciler_statusConditions(t *testing.T) { recResult := sreconcile.ResultSuccess var retErr error - summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), patchHelper) + summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(gitRepositoryReadyCondition), + summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), summarize.WithReconcileResult(recResult), summarize.WithReconcileError(retErr), summarize.WithIgnoreNotFound(), @@ -1857,6 +1981,7 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { reconciler := &GitRepositoryReconciler{ EventRecorder: recorder, features: features.FeatureGates(), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } reconciler.notify(ctx, oldObj, newObj, tt.commit, tt.res, tt.resErr) @@ -1996,6 +2121,7 @@ func TestGitRepositoryReconciler_fetchIncludes(t *testing.T) { r := &GitRepositoryReconciler{ Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.GitRepository{