From 7559011bcb4fda141c2f4d2ecb62a801bf8cad32 Mon Sep 17 00:00:00 2001 From: Venkata Krishna Rohit Sakala Date: Mon, 6 May 2024 17:39:55 -0700 Subject: [PATCH 1/3] For clusterRepo updation fails for rancher bundle, we don't error Purpose is not known but copying from rancher@417bdfd#diff-796d989f6c1367daebda4c3f2d5baed13e3ece991060dbcf049a535d9b609cf2R58 --- pkg/catalogv2/git/download.go | 41 ++++++++++------------------------- pkg/catalogv2/git/git.go | 26 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 29 deletions(-) diff --git a/pkg/catalogv2/git/download.go b/pkg/catalogv2/git/download.go index 95fd88e1442..5c84ed1ec4b 100644 --- a/pkg/catalogv2/git/download.go +++ b/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) { diff --git a/pkg/catalogv2/git/git.go b/pkg/catalogv2/git/git.go index fcef58513bd..8924ec65c83 100644 --- a/pkg/catalogv2/git/git.go +++ b/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 From efb3877f8839e2703b5fb7a8c58023cd38307911 Mon Sep 17 00:00:00 2001 From: Venkata Krishna Rohit Sakala Date: Mon, 6 May 2024 17:40:06 -0700 Subject: [PATCH 2/3] Add unit test for the specific logic added --- pkg/catalogv2/git/download_test.go | 38 +++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/pkg/catalogv2/git/download_test.go b/pkg/catalogv2/git/download_test.go index 322cdb23abb..0eb4cd30f4e 100644 --- a/pkg/catalogv2/git/download_test.go +++ b/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)) }) } } From 58083ac7ddaef0fc0bc3befccb614f9ed6b92f93 Mon Sep 17 00:00:00 2001 From: Venkata Krishna Rohit Sakala Date: Mon, 6 May 2024 17:40:14 -0700 Subject: [PATCH 3/3] Remove unused functions --- pkg/catalogv2/git/utils.go | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/pkg/catalogv2/git/utils.go b/pkg/catalogv2/git/utils.go index 6cf209b1c5b..f2636dd2440 100644 --- a/pkg/catalogv2/git/utils.go +++ b/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