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 <hello@hidde.co>

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"

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.
+// checkout process (including transient errors), it records v1beta1.FetchFailedCondition=True and returns early.
-// On a successful checkout it removes v1beta1.CheckoutFailedCondition, and compares the current revision of HEAD to the
+// 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() {

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{
@@ -1320,11 +1320,14 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) {
 
 			r := &GitRepositoryReconciler{
 				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))