chore: improve missing branch error (#587) (#588)

* chore: improve missing branch error (#587)

Signed-off-by: Michael Crenshaw <[email protected]>

* fix lint

Signed-off-by: Michael Crenshaw <[email protected]>

* make test actually test things

Signed-off-by: Michael Crenshaw <[email protected]>

---------

Signed-off-by: Michael Crenshaw <[email protected]>

Author: crenshaw-dev

internal/controller/changetransferpolicy_controller.go

@@ -109,10 +109,11 @@ func (r *ChangeTransferPolicyReconciler) Reconcile(ctx context.Context, req ctrl
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to get git auth provider for ScmProvider %q: %w", scmProvider.GetName(), err)
 	}
-	gitOperations, err := git.NewEnvironmentOperations(ctx, r.Client, gitAuthProvider, ctp.Spec.RepositoryReference, &ctp, ctp.Spec.ActiveBranch)
+	gitRepo, err := utils.GetGitRepositoryFromObjectKey(ctx, r.Client, client.ObjectKey{Namespace: ctp.GetNamespace(), Name: ctp.Spec.RepositoryReference.Name})
 	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed to initialize git client: %w", err)
+		return ctrl.Result{}, fmt.Errorf("failed to get GitRepository: %w", err)
 	}
+	gitOperations := git.NewEnvironmentOperations(gitRepo, gitAuthProvider, ctp.Spec.ActiveBranch)
 
 	// TODO: could probably short circuit the clone and use an ls-remote to compare the sha's of the current ctp status,
 	// this would help with slamming the git provider with clone requests on controller restarts.

internal/git/git.go

@@ -13,7 +13,6 @@ import (
 	"context"
 	_ "embed"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"os"
 	"os/exec"
@@ -24,13 +23,11 @@ import (
 	"github.com/argoproj-labs/gitops-promoter/internal/types/constants"
 	"github.com/relvacode/iso8601"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	"github.com/argoproj-labs/gitops-promoter/api/v1alpha1"
 	"github.com/argoproj-labs/gitops-promoter/internal/metrics"
 	"github.com/argoproj-labs/gitops-promoter/internal/scms"
-	"github.com/argoproj-labs/gitops-promoter/internal/utils"
 	"github.com/argoproj-labs/gitops-promoter/internal/utils/gitpaths"
 )
 
@@ -64,19 +61,12 @@ type HydratorMetadata struct {
 // NewEnvironmentOperations creates a new EnvironmentOperations instance. The activeBranch parameter is used to differentiate
 // between different environments that might use the same GitRepository and avoid conflicts between concurrent
 // operations.
-func NewEnvironmentOperations(ctx context.Context, k8sClient client.Client, gap scms.GitOperationsProvider, repoRef v1alpha1.ObjectReference, obj v1.Object, activeBranch string) (*EnvironmentOperations, error) {
-	gitRepo, err := utils.GetGitRepositoryFromObjectKey(ctx, k8sClient, client.ObjectKey{Namespace: obj.GetNamespace(), Name: repoRef.Name})
-	if err != nil {
-		return nil, fmt.Errorf("failed to get GitRepository: %w", err)
-	}
-
-	gitOperations := EnvironmentOperations{
+func NewEnvironmentOperations(gitRepo *v1alpha1.GitRepository, gap scms.GitOperationsProvider, activeBranch string) *EnvironmentOperations {
+	return &EnvironmentOperations{
 		gap:          gap,
 		gitRepo:      gitRepo,
 		activeBranch: activeBranch,
 	}
-
-	return &gitOperations, nil
 }
 
 // CloneRepo clones the gitRepo to a temporary directory if needed. Does nothing if the repo is already cloned.
@@ -143,46 +133,42 @@ func (g *EnvironmentOperations) GetBranchShas(ctx context.Context, branch string
 	}
 
 	logger.V(4).Info("git path", "path", gitPath)
-	_, stderr, err := g.runCmd(ctx, gitPath, "checkout", "--progress", "-B", branch, "origin/"+branch)
-	if err != nil {
-		logger.Error(err, "could not git checkout", "gitError", stderr)
-		return BranchShas{}, err
-	}
-	logger.V(4).Info("Checked out branch", "branch", branch)
 
+	// Fetch the branch to ensure we have the latest remote ref
 	start := time.Now()
-	_, stderr, err = g.runCmd(ctx, gitPath, "pull", "--progress")
-	metrics.RecordGitOperation(g.gitRepo, metrics.GitOperationPull, metrics.GitOperationResultFromError(err), time.Since(start))
+	_, stderr, err := g.runCmd(ctx, gitPath, "fetch", "origin", branch)
+	metrics.RecordGitOperation(g.gitRepo, metrics.GitOperationFetch, metrics.GitOperationResultFromError(err), time.Since(start))
 	if err != nil {
-		logger.Error(err, "could not git pull", "gitError", stderr)
-		return BranchShas{}, err
+		logger.Error(err, "could not fetch branch", "gitError", stderr)
+		return BranchShas{}, fmt.Errorf("failed to fetch branch %q: %w", branch, err)
 	}
-	logger.V(4).Info("Pulled branch", "branch", branch)
+	logger.V(4).Info("Fetched branch", "branch", branch)
 
-	stdout, stderr, err := g.runCmd(ctx, gitPath, "rev-parse", branch)
+	// Get the SHA of the remote branch
+	stdout, stderr, err := g.runCmd(ctx, gitPath, "rev-parse", "origin/"+branch)
 	if err != nil {
-		logger.Error(err, "could not get branch shas", "gitError", stderr)
-		return BranchShas{}, err
+		logger.Error(err, "could not get branch sha", "gitError", stderr)
+		return BranchShas{}, fmt.Errorf("failed to get SHA for branch %q: %w", branch, err)
 	}
 
 	shas := BranchShas{}
 	shas.Hydrated = strings.TrimSpace(stdout)
 	logger.V(4).Info("Got hydrated branch sha", "branch", branch, "sha", shas.Hydrated)
 
-	// TODO: safe path join
-	metadataFile := gitPath + "/hydrator.metadata"
-	jsonFile, err := os.Open(metadataFile)
+	// Get the metadata file contents directly from the remote branch
+	metadataFileStdout, stderr, err := g.runCmd(ctx, gitPath, "show", "origin/"+branch+":hydrator.metadata")
 	if err != nil {
-		if errors.Is(err, os.ErrNotExist) {
-			logger.Info("hydrator.metadata file not found", "file", metadataFile)
+		if strings.Contains(stderr, "does not exist") || strings.Contains(stderr, "Path not in") {
+			logger.Info("hydrator.metadata file not found", "branch", branch)
 			return shas, nil
 		}
-		return BranchShas{}, fmt.Errorf("could not open metadata file: %w", err)
+		logger.Error(err, "could not get metadata file", "gitError", stderr)
+		return BranchShas{}, fmt.Errorf("failed to read hydrator.metadata from branch %q: %w", branch, err)
 	}
+	logger.V(4).Info("Got metadata file", "branch", branch)
 
 	var hydratorFile HydratorMetadata
-	decoder := json.NewDecoder(jsonFile)
-	err = decoder.Decode(&hydratorFile)
+	err = json.Unmarshal([]byte(metadataFileStdout), &hydratorFile)
 	if err != nil {
 		return BranchShas{}, fmt.Errorf("could not unmarshal metadata file: %w", err)
 	}

internal/git/git_test.go

@@ -0,0 +1,127 @@
+package git_test
+
+import (
+	"context"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/argoproj-labs/gitops-promoter/api/v1alpha1"
+	"github.com/argoproj-labs/gitops-promoter/internal/git"
+)
+
+func TestGit(t *testing.T) {
+	t.Parallel()
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Git Suite")
+}
+
+// Helper function to run git commands
+func runGitCmd(dir string, args ...string) (string, error) {
+	cmd := exec.Command("git", args...)
+	cmd.Dir = dir
+	cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0")
+	output, err := cmd.CombinedOutput()
+	return string(output), err
+}
+
+var _ = Describe("GetBranchShas", func() {
+	var tempRepoDir string
+
+	BeforeEach(func() {
+		// Create a temporary directory for the test repository
+		var err error
+		tempRepoDir, err = os.MkdirTemp("", "git-test-*")
+		Expect(err).NotTo(HaveOccurred())
+	})
+
+	AfterEach(func() {
+		if tempRepoDir != "" {
+			Expect(os.RemoveAll(tempRepoDir)).To(Succeed())
+		}
+	})
+
+	Context("When the branch does not exist on the remote", func() {
+		It("should provide a clear error message from GetBranchShas", func() {
+			By("Setting up a bare git repository")
+			_, err := runGitCmd(tempRepoDir, "init", "--bare")
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Creating an initial commit")
+			workDir, err := os.MkdirTemp("", "git-work-*")
+			Expect(err).NotTo(HaveOccurred())
+			defer func() {
+				Expect(os.RemoveAll(workDir)).To(Succeed())
+			}()
+
+			_, err = runGitCmd(workDir, "clone", tempRepoDir, ".")
+			Expect(err).NotTo(HaveOccurred())
+
+			_, err = runGitCmd(workDir, "config", "user.name", "Test User")
+			Expect(err).NotTo(HaveOccurred())
+			_, err = runGitCmd(workDir, "config", "user.email", "[email protected]")
+			Expect(err).NotTo(HaveOccurred())
+
+			err = os.WriteFile(filepath.Join(workDir, "hydrator.metadata"), []byte(`{"drySha": "abc123"}`), 0o644)
+			Expect(err).NotTo(HaveOccurred())
+			_, err = runGitCmd(workDir, "add", "hydrator.metadata")
+			Expect(err).NotTo(HaveOccurred())
+			_, err = runGitCmd(workDir, "commit", "-m", "Initial commit")
+			Expect(err).NotTo(HaveOccurred())
+
+			defaultBranch, err := runGitCmd(workDir, "rev-parse", "--abbrev-ref", "HEAD")
+			Expect(err).NotTo(HaveOccurred())
+			defaultBranch = strings.TrimSpace(defaultBranch)
+
+			_, err = runGitCmd(workDir, "push", "origin", defaultBranch)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Prepare EnvironmentOperations
+			repo := &v1alpha1.GitRepository{
+				Spec: v1alpha1.GitRepositorySpec{
+					GitHub: &v1alpha1.GitHubRepo{
+						Owner: "test-owner",
+						Name:  "testrepo",
+					},
+					ScmProviderRef: v1alpha1.ScmProviderObjectReference{
+						Kind: "ScmProvider",
+						Name: "testprovider",
+					},
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "testrepo",
+					Namespace: "default",
+				},
+			}
+			gap := &fakeGitProvider{tempDirPath: tempRepoDir}
+			g := git.NewEnvironmentOperations(repo, gap, defaultBranch)
+			Expect(g.CloneRepo(GinkgoT().Context())).To(Succeed())
+
+			// Call GetBranchShas with a non-existent branch
+			_, err = g.GetBranchShas(GinkgoT().Context(), "environments/qal-usw2-eks-next")
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("failed to fetch branch"))
+
+			// Having a missing branch is a common error, so we're ensuring the error message is clear.
+			Expect(err.Error()).To(ContainSubstring("couldn't find remote ref"))
+		})
+	})
+})
+
+type fakeGitProvider struct {
+	tempDirPath string
+}
+
+func (f *fakeGitProvider) GetGitHttpsRepoUrl(repo v1alpha1.GitRepository) string {
+	// Return the local bare repo path for testing
+	return f.tempDirPath
+}
+func (f *fakeGitProvider) GetUser(ctx context.Context) (string, error)  { return "user", nil }
+func (f *fakeGitProvider) GetToken(ctx context.Context) (string, error) { return "token", nil }