From 9c866ee49d44e0049790f936beae7412f689223c Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 30 Nov 2022 18:49:15 +0000 Subject: [PATCH] Add progressive status in helmchart reconciler Signed-off-by: Sunny --- controllers/helmchart_controller.go | 75 +++++--- controllers/helmchart_controller_test.go | 218 ++++++++++++++++++++--- 2 files changed, 244 insertions(+), 49 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index c6a57692..f5cf3f93 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -30,6 +30,8 @@ import ( eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" soci "github.com/fluxcd/source-controller/internal/oci" + "github.com/google/go-containerregistry/pkg/authn" + "github.com/google/go-containerregistry/pkg/v1/remote" helmgetter "helm.sh/helm/v3/pkg/getter" helmreg "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" @@ -56,9 +58,8 @@ 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/untar" - "github.com/google/go-containerregistry/pkg/authn" - "github.com/google/go-containerregistry/pkg/v1/remote" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/cache" @@ -133,6 +134,8 @@ type HelmChartReconciler struct { Cache *cache.Cache TTL time.Duration *cache.CacheRecorder + + patchOptions []patch.Option } func (r *HelmChartReconciler) SetupWithManager(mgr ctrl.Manager) error { @@ -147,9 +150,11 @@ type HelmChartReconcilerOptions struct { // helmChartReconcileFunc is the function type for all the v1beta2.HelmChart // (sub)reconcile functions. The type implementations are grouped and // executed serially to perform the complete reconcile of the object. -type helmChartReconcileFunc func(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) +type helmChartReconcileFunc func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) func (r *HelmChartReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts HelmChartReconcilerOptions) error { + r.patchOptions = getPatchOptions(helmChartReadyCondition.Owned, r.ControllerName) + if err := mgr.GetCache().IndexField(context.TODO(), &sourcev1.HelmRepository{}, sourcev1.HelmRepositoryURLIndexKey, r.indexHelmRepositoryByURL); err != nil { return fmt.Errorf("failed setting index fields: %w", err) @@ -200,10 +205,7 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( 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 @@ -211,7 +213,7 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // Always attempt to patch the object 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(helmChartReadyCondition), summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), @@ -259,19 +261,35 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( r.reconcileSource, r.reconcileArtifact, } - recResult, retErr = r.reconcile(ctx, obj, reconcilers) + recResult, retErr = r.reconcile(ctx, serialPatcher, obj, reconcilers) return } // reconcile iterates through the helmChartReconcileFunc tasks for the // object. It returns early on the first call that returns // reconcile.ResultRequeue, or produces an error. -func (r *HelmChartReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmChart, reconcilers []helmChartReconcileFunc) (sreconcile.Result, error) { +func (r *HelmChartReconciler) reconcile(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, reconcilers []helmChartReconcileFunc) (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 reconcileAtVal string + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + reconcileAtVal = 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 reconcileAtVal != obj.Status.GetLastHandledReconcileRequest(): + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } // Run the sub-reconcilers and build the result of reconciliation. @@ -281,7 +299,7 @@ func (r *HelmChartReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmC resErr error ) for _, rec := range reconcilers { - recResult, err := rec(ctx, obj, &build) + recResult, err := rec(ctx, sp, obj, &build) // Exit immediately on ResultRequeue. if recResult == sreconcile.ResultRequeue { return sreconcile.ResultRequeue, nil @@ -344,22 +362,31 @@ func (r *HelmChartReconciler) notify(ctx context.Context, oldObj, newObj *source // 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 *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) { +func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, build *chart.Build) (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 } @@ -371,7 +398,7 @@ func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev return sreconcile.ResultSuccess, nil } -func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (_ sreconcile.Result, retErr error) { +func (r *HelmChartReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, build *chart.Build) (_ sreconcile.Result, retErr error) { // Remove any failed verification condition. // The reason is that a failing verification should be recalculated. if conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) { @@ -418,7 +445,7 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1 // Defer observation of build result defer func() { // Record both success and error observations on the object - observeChartBuild(obj, build, retErr) + observeChartBuild(ctx, sp, r.patchOptions, obj, build, retErr) // If we actually build a chart, take a historical note of any dependencies we resolved. // The reason this is a done conditionally, is because if we have a cached one in storage, @@ -810,7 +837,7 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj // early. // On a successful archive, the Artifact in the Status of the object is set, // and the symlink in the Storage is updated to its path. -func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmChart, b *chart.Build) (sreconcile.Result, error) { +func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, b *chart.Build) (sreconcile.Result, error) { // Without a complete chart build, there is little to reconcile if !b.Complete() { return sreconcile.ResultRequeue, nil @@ -1265,10 +1292,16 @@ func (r *HelmChartReconciler) eventLogf(ctx context.Context, obj runtime.Object, } // observeChartBuild records the observation on the given given build and error on the object. -func observeChartBuild(obj *sourcev1.HelmChart, build *chart.Build, err error) { +func observeChartBuild(ctx context.Context, sp *patch.SerialPatcher, pOpts []patch.Option, obj *sourcev1.HelmChart, build *chart.Build, err error) { if build.HasMetadata() { if build.Name != obj.Status.ObservedChartName || !obj.GetArtifact().HasRevision(build.Version) { - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewChart", build.Summary()) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewChart", build.Summary()) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", build.Summary()) + if err := sp.Patch(ctx, obj, pOpts...); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to patch") + } } } diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index cd71e1e8..15b2424f 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -309,6 +309,7 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { } } testStorage.SetArtifactURL(obj.Status.Artifact) + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, assertArtifact: &sourcev1.Artifact{ @@ -325,6 +326,17 @@ func TestHelmChartReconciler_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", @@ -341,7 +353,8 @@ func TestHelmChartReconciler_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"), }, }, { @@ -359,6 +372,7 @@ func TestHelmChartReconciler_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, @@ -372,6 +386,9 @@ func TestHelmChartReconciler_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 { @@ -383,20 +400,30 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { }() r := &HelmChartReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmChart{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", + Generation: 1, }, } if tt.beforeFunc != nil { g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed()) } - got, err := r.reconcileStorage(context.TODO(), obj, nil) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileStorage(context.TODO(), sp, obj, nil) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -414,6 +441,10 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { } g.Expect(absoluteP).NotTo(BeAnExistingFile()) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -468,13 +499,51 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.ObservedSourceArtifactRevision).To(Equal(gitArtifact.Revision)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled 'helmchart' chart with version '0.1.0'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), })) }, cleanFunc: func(g *WithT, build *chart.Build) { g.Expect(os.Remove(build.Path)).To(Succeed()) }, }, + { + name: "Existing artifact makes AritfactOutdated=True", + source: &sourcev1.GitRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gitrepository", + Namespace: "default", + }, + Status: sourcev1.GitRepositoryStatus{ + Artifact: gitArtifact, + }, + }, + beforeFunc: func(obj *sourcev1.HelmChart) { + obj.Spec.Chart = "testdata/charts/helmchart-0.1.0.tgz" + obj.Spec.SourceRef = sourcev1.LocalHelmChartSourceReference{ + Name: "gitrepository", + Kind: sourcev1.GitRepositoryKind, + } + obj.Status.Artifact = &sourcev1.Artifact{ + Path: "some-path", + Revision: "some-rev", + } + }, + want: sreconcile.ResultSuccess, + assertFunc: func(g *WithT, build chart.Build, obj sourcev1.HelmChart) { + g.Expect(build.Complete()).To(BeTrue()) + g.Expect(build.Name).To(Equal("helmchart")) + g.Expect(build.Version).To(Equal("0.1.0")) + g.Expect(build.Path).To(BeARegularFile()) + + g.Expect(obj.Status.ObservedSourceArtifactRevision).To(Equal(gitArtifact.Revision)) + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled 'helmchart' chart with version '0.1.0'"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + })) + }, + }, { name: "Error on unavailable source", beforeFunc: func(obj *sourcev1.HelmChart) { @@ -482,6 +551,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { Name: "unavailable", Kind: sourcev1.GitRepositoryKind, } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultEmpty, wantErr: &serror.Event{Err: errors.New("gitrepositories.source.toolkit.fluxcd.io \"unavailable\" not found")}, @@ -490,6 +561,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, "SourceUnavailable", "failed to get source: gitrepositories.source.toolkit.fluxcd.io \"unavailable\" not found"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), })) }, }, @@ -500,6 +573,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { Name: "unavailable", Kind: "Unsupported", } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "foo") }, want: sreconcile.ResultEmpty, wantErr: &serror.Stalling{Err: errors.New("unsupported source kind 'Unsupported'")}, @@ -508,6 +583,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, "SourceUnavailable", "failed to get source: unsupported source kind"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "foo"), })) }, }, @@ -529,6 +606,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { Kind: sourcev1.GitRepositoryKind, } obj.Spec.ValuesFiles = []string{"invalid.yaml"} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "foo") }, want: sreconcile.ResultEmpty, wantErr: &serror.Stalling{Err: errors.New("values files merge error: no values file found at path")}, @@ -537,6 +616,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ValuesFilesError", "values files merge error: no values file found at path"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "foo"), })) }, }, @@ -556,6 +637,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { Kind: sourcev1.GitRepositoryKind, } obj.Status.ObservedSourceArtifactRevision = "foo" + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "foo") }, want: sreconcile.ResultRequeue, assertFunc: func(g *WithT, build chart.Build, obj sourcev1.HelmChart) { @@ -564,6 +647,8 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(obj.Status.ObservedSourceArtifactRevision).To(Equal("foo")) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, "NoSourceArtifact", "no artifact available"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "foo"), })) }, }, @@ -572,7 +657,7 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - clientBuilder := fake.NewClientBuilder() + clientBuilder := fake.NewClientBuilder().WithScheme(testEnv.GetScheme()) if tt.source != nil { clientBuilder.WithRuntimeObjects(tt.source) } @@ -581,12 +666,14 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { Client: clientBuilder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: storage, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := sourcev1.HelmChart{ ObjectMeta: metav1.ObjectMeta{ - Name: "chart", - Namespace: "default", + Name: "chart", + Namespace: "default", + Generation: 1, }, Spec: sourcev1.HelmChartSpec{}, } @@ -599,7 +686,14 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { defer tt.cleanFunc(g, &b) } - got, err := r.reconcileSource(context.TODO(), &obj, &b) + g.Expect(r.Client.Create(context.TODO(), &obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), &obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(&obj, r.Client) + + got, err := r.reconcileSource(context.TODO(), sp, &obj, &b) g.Expect(err != nil).To(Equal(tt.wantErr != nil)) if tt.wantErr != nil { @@ -611,6 +705,10 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { if tt.assertFunc != nil { tt.assertFunc(g, b, obj) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, &obj) }) } } @@ -840,6 +938,7 @@ func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Getters: testGetters, Storage: storage, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } repository := &sourcev1.HelmRepository{ @@ -1006,8 +1105,6 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { assertFunc: func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) { g.Expect(build.Name).To(Equal(metadata.Name)) g.Expect(build.Version).To(Equal(metadata.Version)) - fmt.Println("buildpath", build.Path) - fmt.Println("storage Path", storage.LocalPath(*cachedArtifact.DeepCopy())) g.Expect(build.Path).ToNot(Equal(storage.LocalPath(*cachedArtifact.DeepCopy()))) g.Expect(build.Path).To(BeARegularFile()) }, @@ -1071,6 +1168,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { Getters: testGetters, Storage: storage, RegistryClientGenerator: registry.ClientGenerator, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } repository := &sourcev1.HelmRepository{ @@ -1276,6 +1374,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { Storage: storage, Getters: testGetters, RegistryClientGenerator: registry.ClientGenerator, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmChart{ @@ -1433,8 +1532,10 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { g := NewWithT(t) r := &HelmChartReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmChart{ @@ -1448,7 +1549,14 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { tt.beforeFunc(obj) } - got, err := r.reconcileArtifact(ctx, obj, tt.build) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileArtifact(ctx, sp, obj, tt.build) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) @@ -1477,7 +1585,8 @@ func TestHelmChartReconciler_getHelmRepositorySecret(t *testing.T) { clientBuilder.WithObjects(mock) r := &HelmChartReconciler{ - Client: clientBuilder.Build(), + Client: clientBuilder.Build(), + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } tests := []struct { @@ -1572,7 +1681,8 @@ func TestHelmChartReconciler_getSource(t *testing.T) { clientBuilder.WithObjects(mocks...) r := &HelmChartReconciler{ - Client: clientBuilder.Build(), + Client: clientBuilder.Build(), + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } tests := []struct { @@ -1678,6 +1788,7 @@ func TestHelmChartReconciler_reconcileDelete(t *testing.T) { r := &HelmChartReconciler{ EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmChart{ @@ -1704,7 +1815,7 @@ func TestHelmChartReconciler_reconcileDelete(t *testing.T) { func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { // Helper to build simple helmChartReconcileFunc with result and error. buildReconcileFuncs := func(r sreconcile.Result, e error) helmChartReconcileFunc { - return func(_ context.Context, _ *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { + return func(_ context.Context, _ *patch.SerialPatcher, _ *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { return r, e } } @@ -1725,6 +1836,10 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultSuccess, wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, { name: "successful reconciliation with generation difference", @@ -1736,7 +1851,8 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { wantResult: sreconcile.ResultSuccess, wantErr: false, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewGeneration", "reconciling new object generation (3)"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "processing object: new generation 2 -> 3"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "processing object: new generation 2 -> 3"), }, }, { @@ -1746,15 +1862,19 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultEmpty, wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, { name: "multiple object status conditions mutations", reconcileFuncs: []helmChartReconcileFunc{ - func(_ context.Context, obj *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { + func(_ context.Context, _ *patch.SerialPatcher, obj *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision") return sreconcile.ResultSuccess, nil }, - func(_ context.Context, obj *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { + func(_ context.Context, _ *patch.SerialPatcher, obj *sourcev1.HelmChart, _ *chart.Build) (sreconcile.Result, error) { conditions.MarkTrue(obj, meta.ReconcilingCondition, "Progressing", "creating artifact") return sreconcile.ResultSuccess, nil }, @@ -1763,7 +1883,8 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { wantErr: false, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), - *conditions.TrueCondition(meta.ReconcilingCondition, "Progressing", "creating artifact"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "creating artifact"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), }, }, { @@ -1775,6 +1896,10 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultRequeue, wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, { name: "subrecs with error before result=Requeue", @@ -1785,6 +1910,10 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { }, wantResult: sreconcile.ResultEmpty, wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "reconciliation in progress"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress"), + }, }, } @@ -1792,7 +1921,10 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - r := &HelmChartReconciler{} + r := &HelmChartReconciler{ + Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), + } obj := &sourcev1.HelmChart{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", @@ -1803,7 +1935,14 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { }, } - got, err := r.reconcile(context.TODO(), obj, tt.reconcileFuncs) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcile(context.TODO(), sp, obj, tt.reconcileFuncs) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.wantResult)) @@ -1898,8 +2037,7 @@ func TestHelmChartReconciler_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) @@ -1909,9 +2047,10 @@ func TestHelmChartReconciler_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(helmChartReadyCondition), + summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), summarize.WithReconcileResult(recResult), summarize.WithReconcileError(retErr), summarize.WithIgnoreNotFound(), @@ -2012,6 +2151,7 @@ func TestHelmChartReconciler_notify(t *testing.T) { reconciler := &HelmChartReconciler{ EventRecorder: recorder, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } build := &chart.Build{ Name: "foo", @@ -2061,7 +2201,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { name: "HTTP without basic auth", want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), }, }, { @@ -2075,7 +2216,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { password: testRegistryPassword, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), }, }, { @@ -2114,7 +2256,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { }, provider: "azure", assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), }, }, } @@ -2200,6 +2343,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Getters: testGetters, RegistryClientGenerator: registry.ClientGenerator, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } var b chart.Build @@ -2216,7 +2360,14 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", metadata.Version) } - got, err := r.reconcileSource(ctx, obj, &b) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileSource(ctx, sp, obj, &b) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) @@ -2323,8 +2474,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of version "), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled '' chart with version ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled '' chart with version ''"), }, cleanFunc: func(g *WithT, build *chart.Build) { g.Expect(os.Remove(build.Path)).To(Succeed()) @@ -2342,6 +2494,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled '' chart with version ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled '' chart with version ''"), }, cleanFunc: func(g *WithT, build *chart.Build) { g.Expect(os.Remove(build.Path)).To(Succeed()) @@ -2383,6 +2537,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T Getters: testGetters, Storage: storage, RegistryClientGenerator: registry.ClientGenerator, + patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), } obj := &sourcev1.HelmChart{ @@ -2434,7 +2589,14 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T defer tt.cleanFunc(g, &b) } - got, err := r.reconcileSource(ctx, obj, &b) + g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(context.TODO(), obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileSource(ctx, sp, obj, &b) if tt.wantErr { tt.wantErrMsg = strings.ReplaceAll(tt.wantErrMsg, "", chartUrl) g.Expect(err).ToNot(BeNil())