From 68794ac22a23137a442aa0ccf61e99450d47c8a9 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Mon, 9 Aug 2021 13:24:49 +0200 Subject: [PATCH] Consolidate condition types into `FetchFailed` This commit consolidates the `DownloadFailed` and `CheckoutFailed` Condition types into a new more generic `FetchFailed` type to simplify the API and observations by consumers. Signed-off-by: Hidde Beydals --- api/v1beta2/gitrepository_types.go | 5 ----- controllers/gitrepository_controller.go | 20 +++++++++---------- controllers/gitrepository_controller_test.go | 21 +++++++++++--------- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/api/v1beta2/gitrepository_types.go b/api/v1beta2/gitrepository_types.go index 6ab6f369..8238dce7 100644 --- a/api/v1beta2/gitrepository_types.go +++ b/api/v1beta2/gitrepository_types.go @@ -36,11 +36,6 @@ const ( ) const ( - // CheckoutFailedCondition indicates a transient or persistent checkout failure. If True, observations on the - // upstream Source revision are not possible, and the Artifact available for the Source may be outdated. - // This is a "negative polarity" or "abnormal-true" type, and is only present on the resource if it is True. - CheckoutFailedCondition string = "CheckoutFailed" - // SourceVerifiedCondition indicates the integrity of the Source has been verified. If True, the integrity check // succeeded. If False, it failed. The Condition is only present on the resource if the integrity has been verified. SourceVerifiedCondition string = "SourceVerified" diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 8aae01dd..88d662f3 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -123,13 +123,13 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques conditions.WithConditions( sourcev1.IncludeUnavailableCondition, sourcev1.SourceVerifiedCondition, - sourcev1.CheckoutFailedCondition, + sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, sourcev1.ArtifactUnavailableCondition, ), conditions.WithNegativePolarityConditions( sourcev1.ArtifactUnavailableCondition, - sourcev1.CheckoutFailedCondition, + sourcev1.FetchFailedCondition, sourcev1.SourceVerifiedCondition, sourcev1.IncludeUnavailableCondition, sourcev1.ArtifactOutdatedCondition, @@ -141,7 +141,7 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques patch.WithOwnedConditions{ Conditions: []string{ sourcev1.ArtifactUnavailableCondition, - sourcev1.CheckoutFailedCondition, + sourcev1.FetchFailedCondition, sourcev1.IncludeUnavailableCondition, sourcev1.ArtifactOutdatedCondition, meta.ReadyCondition, @@ -272,8 +272,8 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sou // and observes its state. // // The repository is checked out to the given dir using the defined configuration, and in case of an error during the -// checkout process (including transient errors), it records v1beta1.CheckoutFailedCondition=True and returns early. -// On a successful checkout it removes v1beta1.CheckoutFailedCondition, and compares the current revision of HEAD to the +// checkout process (including transient errors), it records v1beta1.FetchFailedCondition=True and returns early. +// On a successful checkout it removes v1beta1.FetchFailedCondition, and compares the current revision of HEAD to the // artifact on the object, and records v1beta1.ArtifactOutdatedCondition if they differ. // If instructed, the signature of the commit is verified if and recorded as v1beta1.SourceVerifiedCondition. If the // signature can not be verified or the verification fails, the Condition=False and it returns early. @@ -294,7 +294,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, } var secret corev1.Secret if err := r.Client.Get(ctx, name, &secret); err != nil { - conditions.MarkTrue(obj, sourcev1.CheckoutFailedCondition, sourcev1.AuthenticationFailedReason, + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "Failed to get secret '%s': %s", name.String(), err.Error()) r.Eventf(obj, events.EventSeverityError, sourcev1.AuthenticationFailedReason, "Failed to get secret '%s': %s", name.String(), err.Error()) @@ -309,7 +309,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, authOpts, err = git.AuthOptionsWithoutSecret(obj.Spec.URL) } if err != nil { - conditions.MarkTrue(obj, sourcev1.CheckoutFailedCondition, sourcev1.AuthenticationFailedReason, + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "Failed to configure auth strategy for Git implementation '%s': %s", obj.Spec.GitImplementation, err) r.Eventf(obj, events.EventSeverityError, sourcev1.AuthenticationFailedReason, "Failed to configure auth strategy for Git implementation '%s': %s", obj.Spec.GitImplementation, err) @@ -329,7 +329,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, git.Implementation(obj.Spec.GitImplementation), checkoutOpts) if err != nil { ctrl.LoggerFrom(ctx).Error(err, fmt.Sprintf("Failed to configure checkout strategy for Git implementation '%s'", obj.Spec.GitImplementation)) - conditions.MarkTrue(obj, sourcev1.CheckoutFailedCondition, sourcev1.GitOperationFailedReason, + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "Failed to configure checkout strategy for Git implementation '%s': %s", obj.Spec.GitImplementation, err) // Do not return err as recovery without changes is impossible return ctrl.Result{}, nil @@ -340,7 +340,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, defer cancel() commit, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts) if err != nil { - conditions.MarkTrue(obj, sourcev1.CheckoutFailedCondition, sourcev1.GitOperationFailedReason, + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "Failed to checkout and determine revision: %s", err) r.Eventf(obj, events.EventSeverityError, sourcev1.GitOperationFailedReason, "Failed to checkout and determine revision: %s", err) @@ -349,7 +349,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, } r.Eventf(obj, events.EventSeverityInfo, sourcev1.GitOperationSucceedReason, "Cloned repository '%s' and checked out revision '%s'", obj.Spec.URL, commit.String()) - conditions.Delete(obj, sourcev1.CheckoutFailedCondition) + conditions.Delete(obj, sourcev1.FetchFailedCondition) // Verify commit signature if result, err := r.verifyCommitSignature(ctx, obj, *commit); err != nil || result.IsZero() { diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index f356e3af..96b1b9ce 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -298,7 +298,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.CheckoutFailedCondition, sourcev1.GitOperationFailedReason, "x509: certificate signed by unknown authority"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "x509: certificate signed by unknown authority"), }, }, { @@ -323,7 +323,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.CheckoutFailedCondition, sourcev1.GitOperationFailedReason, "Failed to checkout and determine revision: unable to clone: Certificate"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "Failed to checkout and determine revision: unable to clone: Certificate"), }, }, { @@ -384,7 +384,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.CheckoutFailedCondition, "AuthenticationFailed", "Failed to get secret '/non-existing': secrets \"non-existing\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "Failed to get secret '/non-existing': secrets \"non-existing\" not found"), }, }, } @@ -1259,7 +1259,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { { name: "mixed failed conditions", beforeFunc: func(obj *sourcev1.GitRepository) { - conditions.MarkTrue(obj, sourcev1.CheckoutFailedCondition, "Foo", "") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "Foo", "") conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "Foo", "") conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, "Foo", "") conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "") @@ -1274,7 +1274,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { name: "reconciling and failed conditions", beforeFunc: func(obj *sourcev1.GitRepository) { conditions.MarkTrue(obj, meta.ReconcilingCondition, "Foo", "") - conditions.MarkTrue(obj, sourcev1.CheckoutFailedCondition, "Foo", "") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "Foo", "") }, want: ctrl.Result{RequeueAfter: interval}, assertConditions: []metav1.Condition{ @@ -1285,7 +1285,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { name: "stalled and failed conditions", beforeFunc: func(obj *sourcev1.GitRepository) { conditions.MarkTrue(obj, meta.StalledCondition, "Foo", "") - conditions.MarkTrue(obj, sourcev1.CheckoutFailedCondition, "Foo", "") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "Foo", "") }, want: ctrl.Result{RequeueAfter: interval}, assertConditions: []metav1.Condition{ @@ -1319,12 +1319,15 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).WithObjects(obj) r := &GitRepositoryReconciler{ - Client: builder.Build(), - Storage: testStorage, + Client: builder.Build(), + EventRecorder: record.NewFakeRecorder(32), + Storage: testStorage, } key := client.ObjectKeyFromObject(obj) - res, err := r.Reconcile(logr.NewContext(ctx, log.NullLogger{}), ctrl.Request{NamespacedName: key}) + dlog := log.NewDelegatingLogSink(log.NullLogSink{}) + nullLogger := logr.New(dlog) + res, err := r.Reconcile(logr.NewContext(ctx, nullLogger), ctrl.Request{NamespacedName: key}) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(res).To(Equal(tt.want))