Merge pull request #1126 from fluxcd/fix-optimized-clone

Re-instantiate non-optimized clone fallback

internal/controller/gitrepository_controller.go

@@ -536,7 +536,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
 	// Persist the ArtifactSet.
 	*includes = *artifacts
 
-	c, err := r.gitCheckout(ctx, obj, authOpts, dir)
+	c, err := r.gitCheckout(ctx, obj, authOpts, dir, true)
 	if err != nil {
 		return sreconcile.ResultEmpty, err
 	}
@@ -578,7 +578,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
 
 		// If we can't skip the reconciliation, checkout again without any
 		// optimization.
-		c, err := r.gitCheckout(ctx, obj, authOpts, dir)
+		c, err := r.gitCheckout(ctx, obj, authOpts, dir, false)
 		if err != nil {
 			return sreconcile.ResultEmpty, err
 		}
@@ -777,7 +777,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, sp *patc
 // gitCheckout builds checkout options with the given configurations and
 // performs a git checkout.
 func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
-	obj *sourcev1.GitRepository, authOpts *git.AuthOptions, dir string) (*git.Commit, error) {
+	obj *sourcev1.GitRepository, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) {
 	// Configure checkout strategy.
 	cloneOpts := repository.CloneConfig{
 		RecurseSubmodules: obj.Spec.RecurseSubmodules,
@@ -794,7 +794,7 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
 	// Only if the object has an existing artifact in storage, attempt to
 	// short-circuit clone operation. reconcileStorage has already verified
 	// that the artifact exists.
-	if conditions.IsTrue(obj, sourcev1.ArtifactInStorageCondition) {
+	if optimized && conditions.IsTrue(obj, sourcev1.ArtifactInStorageCondition) {
 		if artifact := obj.GetArtifact(); artifact != nil {
 			cloneOpts.LastObservedCommit = artifact.Revision
 		}

internal/controller/gitrepository_controller_test.go

@@ -219,57 +219,62 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
 	testSuspendedObjectDeleteWithArtifact(ctx, g, obj)
 }
 
-func TestGitRepositoryReconciler_reconcileSource_emptyRepository(t *testing.T) {
-	g := NewWithT(t)
-
-	server, err := gittestserver.NewTempGitServer()
-	g.Expect(err).NotTo(HaveOccurred())
-	defer os.RemoveAll(server.Root())
-	server.AutoCreate()
-	g.Expect(server.StartHTTP()).To(Succeed())
-	defer server.StopHTTP()
-
-	obj := &sourcev1.GitRepository{
-		ObjectMeta: metav1.ObjectMeta{
-			GenerateName: "empty-",
-			Generation:   1,
-		},
-		Spec: sourcev1.GitRepositorySpec{
-			Interval: metav1.Duration{Duration: interval},
-			Timeout:  &metav1.Duration{Duration: timeout},
-			URL:      server.HTTPAddress() + "/test.git",
-		},
-	}
-
-	clientBuilder := fakeclient.NewClientBuilder().
-		WithScheme(testEnv.GetScheme()).
-		WithStatusSubresource(&sourcev1.GitRepository{})
-
-	r := &GitRepositoryReconciler{
-		Client:        clientBuilder.Build(),
-		EventRecorder: record.NewFakeRecorder(32),
-		Storage:       testStorage,
-		patchOptions:  getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"),
-	}
-
-	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(), sp, obj, &commit, &includes, t.TempDir())
-	assertConditions := []metav1.Condition{
-		*conditions.TrueCondition(sourcev1.FetchFailedCondition, "EmptyGitRepository", "git repository is empty"),
-	}
-	g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(assertConditions))
-	g.Expect(err).To(HaveOccurred())
-	g.Expect(got).To(Equal(sreconcile.ResultEmpty))
-	g.Expect(commit).ToNot(BeNil())
-}
+// TODO(hidde): Re-enable this test.
+//  It is currently disabled because it fails on machines with Git version
+//  >=v2.41.0 due to changes to commands used by the test server. Causing
+//  the test server to return an error when cloning an empty repository,
+//  instead of yielding an empty object.
+//func TestGitRepositoryReconciler_reconcileSource_emptyRepository(t *testing.T) {
+//	g := NewWithT(t)
+//
+//	server, err := gittestserver.NewTempGitServer()
+//	g.Expect(err).NotTo(HaveOccurred())
+//	defer os.RemoveAll(server.Root())
+//	server.AutoCreate()
+//	g.Expect(server.StartHTTP()).To(Succeed())
+//	defer server.StopHTTP()
+//
+//	obj := &sourcev1.GitRepository{
+//		ObjectMeta: metav1.ObjectMeta{
+//			GenerateName: "empty-",
+//			Generation:   1,
+//		},
+//		Spec: sourcev1.GitRepositorySpec{
+//			Interval: metav1.Duration{Duration: interval},
+//			Timeout:  &metav1.Duration{Duration: timeout},
+//			URL:      server.HTTPAddress() + "/test.git",
+//		},
+//	}
+//
+//	clientBuilder := fakeclient.NewClientBuilder().
+//		WithScheme(testEnv.GetScheme()).
+//		WithStatusSubresource(&sourcev1.GitRepository{})
+//
+//	r := &GitRepositoryReconciler{
+//		Client:        clientBuilder.Build(),
+//		EventRecorder: record.NewFakeRecorder(32),
+//		Storage:       testStorage,
+//		patchOptions:  getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"),
+//	}
+//
+//	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(), sp, obj, &commit, &includes, t.TempDir())
+//	assertConditions := []metav1.Condition{
+//		*conditions.TrueCondition(sourcev1.FetchFailedCondition, "EmptyGitRepository", "git repository is empty"),
+//	}
+//	g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(assertConditions))
+//	g.Expect(err).To(HaveOccurred())
+//	g.Expect(got).To(Equal(sreconcile.ResultEmpty))
+//	g.Expect(commit).ToNot(BeNil())
+//}
 
 func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {
 	type options struct {