Merge pull request #177 from fluxcd/commit-to-merged-branch

Use libgit2 for clone, fetch, push

controllers/git_error_test.go

@@ -74,29 +74,3 @@ func TestLibgit2ErrorUnchanged(t *testing.T) {
 		t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage)
 	}
 }
-
-func TestGoGitErrorReplace(t *testing.T) {
-	// this is what go-git uses as the error message is if the remote
-	// sends a blank first line
-	unknownMessage := `unknown error: remote: `
-	err := errors.New(unknownMessage)
-	err = gogitPushError(err)
-	reformattedMessage := err.Error()
-	if reformattedMessage == unknownMessage {
-		t.Errorf("expected rewritten error, got %q", reformattedMessage)
-	}
-}
-
-func TestGoGitErrorUnchanged(t *testing.T) {
-	// this is (roughly) what GitHub sends if the deploy key doesn't
-	// have write access; go-git passes this on verbatim
-	regularMessage := `remote: ERROR: deploy key does not have write access`
-	expectedReformat := regularMessage
-	err := errors.New(regularMessage)
-	err = gogitPushError(err)
-	reformattedMessage := err.Error()
-	// test that it's been rewritten, without checking the exact content
-	if len(reformattedMessage) > len(expectedReformat) {
-		t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage)
-	}
-}

controllers/imageupdateautomation_controller.go

@@ -34,7 +34,6 @@ import (
 
 	"github.com/ProtonMail/go-crypto/openpgp"
 	securejoin "github.com/cyphar/filepath-securejoin"
-	"github.com/go-git/go-git/v5/config"
 	"github.com/go-git/go-git/v5/plumbing"
 	"github.com/go-git/go-git/v5/plumbing/object"
 	"github.com/go-logr/logr"
@@ -214,15 +213,15 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 	}
 
 	var repo *gogit.Repository
-	if repo, err = cloneInto(ctx, access, ref, tmp, origin.Spec.GitImplementation); err != nil {
+	if repo, err = cloneInto(ctx, access, ref, tmp); err != nil {
 		return failWithError(err)
 	}
 
 	// When there's a push spec, the pushed-to branch is where commits
 	// shall be made
 
 	if gitSpec.Push != nil {
-		if err := fetch(ctx, tmp, repo, pushBranch, access, origin.Spec.GitImplementation); err != nil && err != errRemoteBranchMissing {
+		if err := fetch(ctx, tmp, pushBranch, access); err != nil && err != errRemoteBranchMissing {
 			return failWithError(err)
 		}
 		if err = switchBranch(repo, pushBranch); err != nil {
@@ -307,7 +306,7 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 			return failWithError(err)
 		}
 	} else {
-		if err := push(ctx, tmp, repo, pushBranch, access, origin.Spec.GitImplementation); err != nil {
+		if err := push(ctx, tmp, pushBranch, access); err != nil {
 			return failWithError(err)
 		}
 
@@ -424,6 +423,10 @@ func (r *ImageUpdateAutomationReconciler) automationsForImagePolicy(obj client.O
 
 // --- git ops
 
+// Note: libgit2 is always used for network operations; for cloning,
+// it will do a non-shallow clone, and for anything else, it doesn't
+// matter what is used.
+
 type repoAccess struct {
 	auth *git.Auth
 	url  string
@@ -433,19 +436,21 @@ func (r *ImageUpdateAutomationReconciler) getRepoAccess(ctx context.Context, rep
 	var access repoAccess
 	access.auth = &git.Auth{}
 	access.url = repository.Spec.URL
-	authStrat, err := gitstrat.AuthSecretStrategyForURL(access.url, git.CheckoutOptions{GitImplementation: repository.Spec.GitImplementation})
+
+	authStrat, err := gitstrat.AuthSecretStrategyForURL(access.url, git.CheckoutOptions{GitImplementation: sourcev1.LibGit2Implementation})
 	if err != nil {
 		return access, err
 	}
 
 	if repository.Spec.SecretRef != nil && authStrat != nil {
+
 		name := types.NamespacedName{
 			Namespace: repository.GetNamespace(),
 			Name:      repository.Spec.SecretRef.Name,
 		}
 
 		var secret corev1.Secret
-		err := r.Client.Get(ctx, name, &secret)
+		err = r.Client.Get(ctx, name, &secret)
 		if err != nil {
 			err = fmt.Errorf("auth secret error: %w", err)
 			return access, err
@@ -468,11 +473,10 @@ func (r repoAccess) remoteCallbacks() libgit2.RemoteCallbacks {
 }
 
 // cloneInto clones the upstream repository at the `ref` given (which
-// can be `nil`), using the git library indicated by `impl`. It
-// returns a `*gogit.Repository` regardless of the git library, since
-// that is used for committing changes.
-func cloneInto(ctx context.Context, access repoAccess, ref *sourcev1.GitRepositoryRef, path, impl string) (*gogit.Repository, error) {
-	checkoutStrat, err := gitstrat.CheckoutStrategyForRef(ref, git.CheckoutOptions{GitImplementation: impl})
+// can be `nil`). It returns a `*gogit.Repository` since that is used
+// for committing changes.
+func cloneInto(ctx context.Context, access repoAccess, ref *sourcev1.GitRepositoryRef, path string) (*gogit.Repository, error) {
+	checkoutStrat, err := gitstrat.CheckoutStrategyForRef(ref, git.CheckoutOptions{GitImplementation: sourcev1.LibGit2Implementation})
 	if err == nil {
 		_, _, err = checkoutStrat.Checkout(ctx, path, access.url, access.auth)
 	}
@@ -490,18 +494,12 @@ func switchBranch(repo *gogit.Repository, pushBranch string) error {
 	localBranch := plumbing.NewBranchReferenceName(pushBranch)
 
 	// is the branch already present?
-	_, err := repo.Reference(localBranch, false)
+	_, err := repo.Reference(localBranch, true)
+	var create bool
 	switch {
 	case err == plumbing.ErrReferenceNotFound:
 		// make a new branch, starting at HEAD
-		head, err := repo.Head()
-		if err != nil {
-			return err
-		}
-		branchRef := plumbing.NewHashReference(localBranch, head.Hash())
-		if err = repo.Storer.SetReference(branchRef); err != nil {
-			return err
-		}
+		create = true
 	case err != nil:
 		return err
 	default:
@@ -516,6 +514,7 @@ func switchBranch(repo *gogit.Repository, pushBranch string) error {
 
 	return tree.Checkout(&gogit.CheckoutOptions{
 		Branch: localBranch,
+		Create: create,
 	})
 }
 
@@ -608,23 +607,12 @@ var errRemoteBranchMissing = errors.New("remote branch missing")
 // returns errRemoteBranchMissing (this is to work in sympathy with
 // `switchBranch`, which will create the branch if it doesn't
 // exist). For any other problem it will return the error.
-func fetch(ctx context.Context, path string, repo *gogit.Repository, branch string, access repoAccess, impl string) error {
+func fetch(ctx context.Context, path string, branch string, access repoAccess) error {
 	refspec := fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch)
-	switch impl {
-	case sourcev1.LibGit2Implementation:
-		lg2repo, err := libgit2.OpenRepository(path)
-		if err != nil {
-			return err
-		}
-		return fetchLibgit2(lg2repo, refspec, access)
-	case sourcev1.GoGitImplementation:
-		return fetchGoGit(ctx, repo, refspec, access)
-	default:
-		return fmt.Errorf("unknown git implementation %q", impl)
+	repo, err := libgit2.OpenRepository(path)
+	if err != nil {
+		return err
 	}
-}
-
-func fetchLibgit2(repo *libgit2.Repository, refspec string, access repoAccess) error {
 	origin, err := repo.Remotes.Lookup(originRemote)
 	if err != nil {
 		return err
@@ -641,69 +629,15 @@ func fetchLibgit2(repo *libgit2.Repository, refspec string, access repoAccess) e
 	return err
 }
 
-func fetchGoGit(ctx context.Context, repo *gogit.Repository, refspec string, access repoAccess) error {
-	err := repo.FetchContext(ctx, &gogit.FetchOptions{
-		RemoteName: originRemote,
-		RefSpecs:   []config.RefSpec{config.RefSpec(refspec)},
-		Auth:       access.auth.AuthMethod,
-	})
-	if err == gogit.NoErrAlreadyUpToDate {
-		return nil
-	}
-	if _, ok := err.(gogit.NoMatchingRefSpecError); ok {
-		return errRemoteBranchMissing
-	}
-	return err
-}
-
 // push pushes the branch given to the origin using the git library
 // indicated by `impl`. It's passed both the path to the repo and a
 // gogit.Repository value, since the latter may as well be used if the
 // implementation is GoGit.
-func push(ctx context.Context, path string, repo *gogit.Repository, branch string, access repoAccess, impl string) error {
-	switch impl {
-	case sourcev1.LibGit2Implementation:
-		lg2repo, err := libgit2.OpenRepository(path)
-		if err != nil {
-			return err
-		}
-		return pushLibgit2(lg2repo, access, branch)
-	case sourcev1.GoGitImplementation:
-		return pushGoGit(ctx, repo, access, branch)
-	default:
-		return fmt.Errorf("unknown git implementation %q", impl)
-	}
-}
-
-func pushGoGit(ctx context.Context, repo *gogit.Repository, access repoAccess, branch string) error {
-	refspec := config.RefSpec(fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch))
-	err := repo.PushContext(ctx, &gogit.PushOptions{
-		RemoteName: originRemote,
-		Auth:       access.auth.AuthMethod,
-		RefSpecs:   []config.RefSpec{refspec},
-	})
-	return gogitPushError(err)
-}
-
-func gogitPushError(err error) error {
-	if err == nil {
-		return nil
-	}
-	switch strings.TrimSpace(err.Error()) {
-	case "unknown error: remote:":
-		// this unhelpful error arises because go-git takes the first
-		// line of the output on stderr, and for some git providers
-		// (GitLab, at least) the output has a blank line at the
-		// start. The rest of stderr is thrown away, so we can't get
-		// the actual error; but at least we know what was being
-		// attempted, and the likely cause.
-		return fmt.Errorf("push rejected; check git secret has write access")
-	default:
+func push(ctx context.Context, path, branch string, access repoAccess) error {
+	repo, err := libgit2.OpenRepository(path)
+	if err != nil {
 		return err
 	}
-}
-
-func pushLibgit2(repo *libgit2.Repository, access repoAccess, branch string) error {
 	origin, err := repo.Remotes.Lookup(originRemote)
 	if err != nil {
 		return err

controllers/update_test.go

@@ -782,6 +782,29 @@ Images:
 					Expect(head.String()).NotTo(Equal(headHash))
 				})
 
+				It("still pushes to the push branch after it's merged", func() {
+					// observe the first commit
+					waitForNewHead(localRepo, pushBranch)
+					head, err := localRepo.Reference(plumbing.NewRemoteReferenceName(originRemote, pushBranch), true)
+					headHash := head.String()
+					Expect(err).NotTo(HaveOccurred())
+
+					// merge the push branch into checkout branch, and push the merge commit
+					// upstream.
+					// waitForNewHead() leaves the repo at the head of the branch given, i.e., the
+					// push branch), so we have to check out the "main" branch first.
+					Expect(checkoutBranch(localRepo, branch)).To(Succeed())
+					mergeBranchIntoHead(localRepo, pushBranch)
+
+					// update the policy and expect another commit in the push branch
+					policy.Status.LatestImage = "helloworld:v1.3.0"
+					Expect(k8sClient.Status().Update(context.TODO(), policy)).To(Succeed())
+					waitForNewHead(localRepo, pushBranch)
+					head, err = localRepo.Reference(plumbing.NewRemoteReferenceName(originRemote, pushBranch), true)
+					Expect(err).NotTo(HaveOccurred())
+					Expect(head.String()).NotTo(Equal(headHash))
+				})
+
 				AfterEach(func() {
 					Expect(k8sClient.Delete(context.Background(), update)).To(Succeed())
 				})
@@ -1044,6 +1067,7 @@ func waitForNewHead(repo *git.Repository, branch string) {
 	// remote, so it is a detached head.
 	Expect(working.Reset(&git.ResetOptions{
 		Commit: remoteHead.Hash(),
+		Mode:   git.HardReset,
 	})).To(Succeed())
 }
 
@@ -1128,3 +1152,57 @@ func initGitRepo(gitServer *gittestserver.GitServer, fixture, branch, repository
 		RefSpecs: []config.RefSpec{"refs/heads/*:refs/heads/*"},
 	})
 }
+
+func checkoutBranch(repo *git.Repository, branch string) error {
+	working, err := repo.Worktree()
+	if err != nil {
+		return err
+	}
+	// check that there's no local changes, as a sanity check
+	status, err := working.Status()
+	if err != nil {
+		return err
+	}
+	if len(status) > 0 {
+		for path := range status {
+			println(path, "is changed")
+		}
+	} // the checkout next will fail if there are changed files
+
+	if err = working.Checkout(&git.CheckoutOptions{
+		Branch: plumbing.NewBranchReferenceName(branch),
+		Create: false,
+	}); err != nil {
+		return err
+	}
+	return nil
+}
+
+// This merges the push branch into HEAD, and pushes upstream. This is
+// to simulate e.g., a PR being merged.
+func mergeBranchIntoHead(repo *git.Repository, pushBranch string) {
+	// hash of head
+	headRef, err := repo.Head()
+	Expect(err).NotTo(HaveOccurred())
+	pushBranchRef, err := repo.Reference(plumbing.NewRemoteReferenceName(originRemote, pushBranch), false)
+	Expect(err).NotTo(HaveOccurred())
+
+	// You need the worktree to be able to create a commit
+	worktree, err := repo.Worktree()
+	Expect(err).NotTo(HaveOccurred())
+	_, err = worktree.Commit(fmt.Sprintf("Merge %s", pushBranch), &git.CommitOptions{
+		Author: &object.Signature{
+			Name:  "Testbot",
+			Email: "test@example.com",
+			When:  time.Now(),
+		},
+		Parents: []plumbing.Hash{headRef.Hash(), pushBranchRef.Hash()},
+	})
+	Expect(err).NotTo(HaveOccurred())
+
+	// push upstream
+	err = repo.Push(&git.PushOptions{
+		RemoteName: originRemote,
+	})
+	Expect(err).NotTo(HaveOccurred())
+}

docs/spec/v1alpha2/imageupdateautomations.md

@@ -70,12 +70,11 @@ type SourceReference struct {
 
 To be able to commit changes back, the referenced `GitRepository` object must refer to credentials
 with write access; e.g., if using a GitHub deploy key, "Allow write access" should be checked when
-creating it. Only the `url`, `ref`, `secretRef` and `gitImplementation` (see just below) fields of
-the `GitRepository` are used.
+creating it. Only the `url`, `ref`, and `secretRef` fields of the `GitRepository` are used.
 
-The `gitImplementation` field in the referenced `GitRepository` object controls which git library is
-used. This will matter if you run on Azure, and possibly otherwise -- see [the source controller
-documentation][source-docs] for more details.
+The [`gitImplementation` field][source-docs] in the referenced `GitRepository` is ignored. The
+automation controller cannot use shallow clones or submodules, so there is no reason to use the
+go-git implementation rather than libgit2.
 
 Other fields particular to how the Git repository is used are in the `git` field, [described
 below](#git-specific-specification).