[v2.8] For clusterRepo updation fails for rancher bundle, we don't error

pkg/catalogv2/git/download.go

@@ -2,6 +2,7 @@ package git
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/rancher/rancher/pkg/settings"
 	corev1 "k8s.io/api/core/v1"
@@ -57,47 +58,29 @@ func Head(secret *corev1.Secret, namespace, name, gitURL, branch string, insecur
 	return commit, nil
 }
 
-// Update updates git repo if remote sha has changed
+// Update updates git repo if remote sha has changed. It also skips the update if in bundled mode and the git dir has a certain prefix stateDir(utils.go).
+// If there is an error updating the repo especially stateDir(utils.go) repositories, it ignores the error and returns the current commit in the local pod directory.
+// except when the error is `branch not found`. It specifically checks for `couldn't find remote ref` & `Could not find remote branch` in the error message.
 func Update(secret *corev1.Secret, namespace, name, gitURL, branch string, insecureSkipTLS bool, caBundle []byte) (string, error) {
 	git, err := gitForRepo(secret, namespace, name, gitURL, insecureSkipTLS, caBundle)
 	if err != nil {
 		return "", fmt.Errorf("update failure: %w", err)
 	}
-
 	if IsBundled(git.Directory) && settings.SystemCatalog.Get() == "bundled" {
 		return Head(secret, namespace, name, gitURL, branch, insecureSkipTLS, caBundle)
 	}
 
-	if err := git.clone(branch); err != nil {
-		return "", nil
-	}
-
-	if err := git.reset("HEAD"); err != nil {
-		return "", fmt.Errorf("update failure: %w", err)
-	}
-
-	commit, err := git.currentCommit()
-	if err != nil {
-		return commit, fmt.Errorf("update failure: %w", err)
-	}
-
-	changed, err := git.remoteSHAChanged(branch, commit)
-	if err != nil {
-		return commit, fmt.Errorf("update failure: %w", err)
-	}
-	if !changed {
-		return commit, nil
-	}
-
-	if err := git.fetchAndReset(branch); err != nil {
-		return "", fmt.Errorf("update failure: %w", err)
-	}
-
-	lastCommit, err := git.currentCommit()
+	commit, err := git.Update(branch)
 	if err != nil && IsBundled(git.Directory) {
+		// We don't report an error unless the branch is invalid
+		// The reason being it would break airgap environments in downstream
+		// cluster. A new issue is created to tackle this in the forthcoming.
+		if strings.Contains(err.Error(), "couldn't find remote ref") || strings.Contains(err.Error(), "Could not find remote branch") {
+			return "", err
+		}
 		return Head(secret, namespace, name, gitURL, branch, insecureSkipTLS, caBundle)
 	}
-	return lastCommit, nil
+	return commit, err
 }
 
 func gitForRepo(secret *corev1.Secret, namespace, name, gitURL string, insecureSkipTLS bool, caBundle []byte) (*git, error) {

pkg/catalogv2/git/download_test.go

@@ -1,6 +1,7 @@
 package git
 
 import (
+	"fmt"
 	"os"
 	"testing"
 
@@ -143,7 +144,8 @@ func Test_Update(t *testing.T) {
 		branch            string
 		systemCatalogMode string
 		expectedCommit    string
-		expectedError     error
+		expectedError     string
+		dir               string
 	}{
 		{
 			test:              "#1 TestCase: Success ",
@@ -156,24 +158,38 @@ func Test_Update(t *testing.T) {
 			branch:            lastBranch,
 			systemCatalogMode: "",
 			expectedCommit:    "226d544def39de56db210e96d2b0b535badf9bdd",
-			expectedError:     nil,
+			expectedError:     "",
+		},
+		{
+			test:              "Returns an error if invalid branch is specified",
+			secret:            nil,
+			namespace:         "cattle-test",
+			name:              "small-fork-test",
+			gitURL:            chartsSmallForkURL,
+			insecureSkipTLS:   false,
+			caBundle:          []byte{},
+			branch:            "invalidbranch",
+			systemCatalogMode: "",
+			expectedCommit:    "226d544def39de56db210e96d2b0b535badf9bdd",
+			expectedError:     "Could not find remote branch",
+			dir:               fmt.Sprintf("%s/%s", localDir, "cattle-test/small-fork-test/d39a2f6abd49e537e5015bbe1a4cd4f14919ba1c3353208a7ff6be37ffe00c52"),
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			commit, err := Update(tc.secret, tc.namespace, tc.name, tc.gitURL, tc.branch, tc.insecureSkipTLS, tc.caBundle)
-			// Check the error
-			if tc.expectedError == nil && tc.expectedError != err {
-				t.Errorf("Expected error: %v |But got: %v", tc.expectedError, err)
+			if tc.dir != "" {
+				err := os.MkdirAll(tc.dir, 0755)
+				assert.NoError(t, err)
 			}
 
-			// Only testing error in some cases
-			if err != nil {
-				assert.EqualError(t, tc.expectedError, err.Error())
+			commit, err := Update(tc.secret, tc.namespace, tc.name, tc.gitURL, tc.branch, tc.insecureSkipTLS, tc.caBundle)
+			if tc.expectedError != "" {
+				assert.Contains(t, err.Error(), tc.expectedError)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, len(commit), len(tc.expectedCommit))
 			}
-
-			assert.Equal(t, len(commit), len(tc.expectedCommit))
 		})
 	}
 }

pkg/catalogv2/git/git.go

@@ -214,6 +214,32 @@ func (g *git) clone(branch string) error {
 	return g.Clone(branch)
 }
 
+// Update updates git repo if remote sha has changed.
+func (g *git) Update(branch string) (string, error) {
+	if err := g.clone(branch); err != nil {
+		return "", err
+	}
+
+	if err := g.reset("HEAD"); err != nil {
+		return "", err
+	}
+
+	commit, err := g.currentCommit()
+	if err != nil {
+		return commit, err
+	}
+
+	if changed, err := g.remoteSHAChanged(branch, commit); err != nil || !changed {
+		return commit, err
+	}
+
+	if err := g.fetchAndReset(branch); err != nil {
+		return "", err
+	}
+
+	return g.currentCommit()
+}
+
 func (g *git) fetchAndReset(rev string) error {
 	if err := g.git("-C", g.Directory, "fetch", "origin", "--", rev); err != nil {
 		return err

pkg/catalogv2/git/utils.go

@@ -11,8 +11,6 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
-
-	"github.com/pkg/errors"
 )
 
 const (
@@ -118,27 +116,6 @@ func formatGitURL(endpoint, branch string) string {
 	return ""
 }
 
-func firstField(lines []string, errText string) (string, error) {
-	if len(lines) == 0 {
-		return "", errors.New(errText)
-	}
-
-	fields := strings.Fields(lines[0])
-	if len(fields) == 0 {
-		return "", errors.New(errText)
-	}
-
-	if len(fields[0]) == 0 {
-		return "", errors.New(errText)
-	}
-
-	return fields[0], nil
-}
-
-func formatRefForBranch(branch string) string {
-	return fmt.Sprintf("refs/heads/%s", branch)
-}
-
 type basicRoundTripper struct {
 	username string
 	password string