From 71d3870e0a5d3b333fc7da4cbf5fb27c14d7caf9 Mon Sep 17 00:00:00 2001 From: Sunny Date: Sat, 14 May 2022 22:58:11 +0530 Subject: [PATCH] Avoid skipping reconciliation - Attempt short-circuiting clone only when the artifact is already in the storage. - A successful no-op clone need not return an error, but a partial commit which contains only a hash + reference. - On no-op clone, reconcileSource() populates the source build dir by copying the existing artifact and lets the reconciliation continue. - Reconciliation is not skipped to allow other subreconcilers to operate on other parts of GitRepo object like include, ignore, etc, when attributes associated with them change but the remote repo has not changed. - Add a function IsConcreteCommit() to differentiate between partial and concrete commit. - Update and simplify go-git and libgit2 no-op clone tests. Signed-off-by: Sunny --- api/v1beta2/condition_types.go | 3 + controllers/gitrepository_controller.go | 48 +++-- controllers/gitrepository_controller_test.go | 69 +++--- docs/spec/v1beta2/gitrepositories.md | 4 +- pkg/git/git.go | 18 +- pkg/git/git_test.go | 39 ++++ pkg/git/gogit/checkout.go | 35 ++- pkg/git/gogit/checkout_test.go | 174 ++++++++------- pkg/git/libgit2/checkout.go | 25 ++- pkg/git/libgit2/checkout_test.go | 211 +++++++++---------- pkg/git/options.go | 3 +- 11 files changed, 370 insertions(+), 259 deletions(-) diff --git a/api/v1beta2/condition_types.go b/api/v1beta2/condition_types.go index 711469eb..4338b237 100644 --- a/api/v1beta2/condition_types.go +++ b/api/v1beta2/condition_types.go @@ -100,4 +100,7 @@ const ( // CacheOperationFailedReason signals a failure in cache operation. CacheOperationFailedReason string = "CacheOperationFailed" + + // CopyOperationFailedReason signals a failure in copying files. + CopyOperationFailedReason string = "CopyOperationFailed" ) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 7c9896a7..acfe53e5 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -425,9 +425,10 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, } if val, ok := r.features[features.OptimizedGitClones]; ok && val { - // Only if the object is ready, use the last revision to attempt - // short-circuiting clone operation. - if conditions.IsTrue(obj, meta.ReadyCondition) { + // 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 artifact := obj.GetArtifact(); artifact != nil { checkoutOpts.LastRevision = artifact.Revision } @@ -478,17 +479,6 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, defer cancel() c, err := checkoutStrategy.Checkout(gitCtx, dir, repositoryURL, authOpts) if err != nil { - var v git.NoChangesError - if errors.As(err, &v) { - // Create generic error without notification. Since it's a no-op - // error, ignore (no runtime error), normal event. - ge := serror.NewGeneric(v, sourcev1.GitOperationSucceedReason) - ge.Notification = false - ge.Ignore = true - ge.Event = corev1.EventTypeNormal - return sreconcile.ResultEmpty, ge - } - e := serror.NewGeneric( fmt.Errorf("failed to checkout and determine revision: %w", err), sourcev1.GitOperationFailedReason, @@ -499,8 +489,38 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, } // Assign the commit to the shared commit reference. *commit = *c + + // If it's a partial commit obtained from an existing artifact, check if the + // reconciliation can be skipped if other configurations have not changed. + if !git.IsConcreteCommit(*commit) { + ctrl.LoggerFrom(ctx).V(logger.DebugLevel).Info(fmt.Sprintf( + "no changes since last reconciliation, observed revision '%s'", commit.String())) + + // Remove the target directory, as CopyToPath() renames another + // directory to which the artifact is unpacked into the target + // directory. At this point, the target directory is empty, safe to + // remove. + os.RemoveAll(dir) + if err := r.Storage.CopyToPath(obj.GetArtifact(), "/", dir); err != nil { + e := serror.NewGeneric( + fmt.Errorf("failed to copy existing artifact to source dir: %w", err), + sourcev1.CopyOperationFailedReason, + ) + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + conditions.Delete(obj, sourcev1.FetchFailedCondition) + conditions.Delete(obj, sourcev1.StorageOperationFailedCondition) + + return sreconcile.ResultSuccess, nil + } + ctrl.LoggerFrom(ctx).V(logger.DebugLevel).Info("git repository checked out", "url", obj.Spec.URL, "revision", commit.String()) conditions.Delete(obj, sourcev1.FetchFailedCondition) + // In case no-op clone resulted in a failure and in the subsequent + // reconciliation a new remote revision was observed, delete any stale + // StorageOperationFailedCondition. + conditions.Delete(obj, sourcev1.StorageOperationFailedCondition) // Verify commit signature if result, err := r.verifyCommitSignature(ctx, obj, *commit); err != nil || result == sreconcile.ResultEmpty { diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index e4ca2fde..27107c29 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -551,27 +551,31 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) want sreconcile.Result wantErr bool wantRevision string + wantArtifactOutdated bool }{ { - name: "Nil reference (default branch)", - want: sreconcile.ResultSuccess, - wantRevision: "master/", + name: "Nil reference (default branch)", + want: sreconcile.ResultSuccess, + wantRevision: "master/", + wantArtifactOutdated: true, }, { name: "Branch", reference: &sourcev1.GitRepositoryRef{ Branch: "staging", }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantArtifactOutdated: true, }, { name: "Tag", reference: &sourcev1.GitRepositoryRef{ Tag: "v0.1.0", }, - want: sreconcile.ResultSuccess, - wantRevision: "v0.1.0/", + want: sreconcile.ResultSuccess, + wantRevision: "v0.1.0/", + wantArtifactOutdated: true, }, { name: "Branch commit", @@ -580,8 +584,9 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) Branch: "staging", Commit: "", }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantArtifactOutdated: true, }, { name: "Branch commit", @@ -590,60 +595,56 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) Branch: "staging", Commit: "", }, - want: sreconcile.ResultSuccess, - wantRevision: "HEAD/", + want: sreconcile.ResultSuccess, + wantRevision: "HEAD/", + wantArtifactOutdated: true, }, { name: "SemVer", reference: &sourcev1.GitRepositoryRef{ SemVer: "*", }, - want: sreconcile.ResultSuccess, - wantRevision: "v2.0.0/", + want: sreconcile.ResultSuccess, + wantRevision: "v2.0.0/", + wantArtifactOutdated: true, }, { name: "SemVer range", reference: &sourcev1.GitRepositoryRef{ SemVer: "", + want: sreconcile.ResultSuccess, + wantRevision: "0.2.0/", + wantArtifactOutdated: true, }, { name: "SemVer prerelease", reference: &sourcev1.GitRepositoryRef{ SemVer: ">=1.0.0-0 <1.1.0-0", }, - wantRevision: "v1.0.0-alpha/", - want: sreconcile.ResultSuccess, + wantRevision: "v1.0.0-alpha/", + want: sreconcile.ResultSuccess, + wantArtifactOutdated: true, }, { - name: "Optimized clone, Ready=True", + name: "Optimized clone", reference: &sourcev1.GitRepositoryRef{ Branch: "staging", }, beforeFunc: func(obj *sourcev1.GitRepository, latestRev string) { + // Add existing artifact on the object and storage. obj.Status = sourcev1.GitRepositoryStatus{ Artifact: &sourcev1.Artifact{ Revision: "staging/" + latestRev, + Path: randStringRunes(10), }, } - conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") + testStorage.Archive(obj.GetArtifact(), "testdata/git/repository", nil) + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultEmpty, - wantErr: true, - wantRevision: "staging/", - }, - { - name: "Optimized clone, Ready=False", - reference: &sourcev1.GitRepositoryRef{ - Branch: "staging", - }, - beforeFunc: func(obj *sourcev1.GitRepository, latestRev string) { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, "not ready") - }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantArtifactOutdated: false, }, } @@ -721,7 +722,7 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) if tt.wantRevision != "" && !tt.wantErr { revision := strings.ReplaceAll(tt.wantRevision, "", headRef.Hash().String()) g.Expect(commit.String()).To(Equal(revision)) - g.Expect(conditions.IsTrue(obj, sourcev1.ArtifactOutdatedCondition)).To(BeTrue()) + g.Expect(conditions.IsTrue(obj, sourcev1.ArtifactOutdatedCondition)).To(Equal(tt.wantArtifactOutdated)) } }) } diff --git a/docs/spec/v1beta2/gitrepositories.md b/docs/spec/v1beta2/gitrepositories.md index 2d95db47..412dd0c2 100644 --- a/docs/spec/v1beta2/gitrepositories.md +++ b/docs/spec/v1beta2/gitrepositories.md @@ -406,8 +406,8 @@ reconciliations. It supports both `go-git` and `libgit2` implementations when cloning repositories using branches or tags. When enabled, avoids full clone operations by first checking whether -the last revision is still the same at the target repository, -and if that is so, skips the reconciliation. +the revision of the last stored artifact is still the head of the remote +repository, and if that is so, the local artifact is reused. This feature is enabled by default. It can be disabled by starting the controller with the argument `--feature-gates=OptimizedGitClones=false`. diff --git a/pkg/git/git.go b/pkg/git/git.go index cc45498d..5ce6fb09 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -107,14 +107,12 @@ type CheckoutStrategy interface { Checkout(ctx context.Context, path, url string, config *AuthOptions) (*Commit, error) } -// NoChangesError represents the case in which a Git clone operation -// is attempted, but cancelled as the revision is still the same as -// the one observed on the last successful reconciliation. -type NoChangesError struct { - Message string - ObservedRevision string -} - -func (e NoChangesError) Error() string { - return fmt.Sprintf("%s: observed revision '%s'", e.Message, e.ObservedRevision) +// IsConcreteCommit returns if a given commit is a concrete commit. Concrete +// commits have most of commit metadata and commit content. In contrast, a +// partial commit may only have some metadata and no commit content. +func IsConcreteCommit(c Commit) bool { + if c.Hash != nil && c.Encoded != nil { + return true + } + return false } diff --git a/pkg/git/git_test.go b/pkg/git/git_test.go index 9d9d94dd..5b67b23b 100644 --- a/pkg/git/git_test.go +++ b/pkg/git/git_test.go @@ -18,6 +18,7 @@ package git import ( "testing" + "time" . "github.com/onsi/gomega" ) @@ -263,3 +264,41 @@ of the commit`, }) } } + +func TestIsConcreteCommit(t *testing.T) { + tests := []struct { + name string + commit Commit + result bool + }{ + { + name: "concrete commit", + commit: Commit{ + Hash: Hash("foo"), + Reference: "refs/tags/main", + Author: Signature{ + Name: "user", Email: "user@example.com", When: time.Now(), + }, + Committer: Signature{ + Name: "user", Email: "user@example.com", When: time.Now(), + }, + Signature: "signature", + Encoded: []byte("commit-content"), + Message: "commit-message", + }, + result: true, + }, + { + name: "partial commit", + commit: Commit{Hash: Hash("foo")}, + result: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(IsConcreteCommit(tt.commit)).To(Equal(tt.result)) + }) + } +} diff --git a/pkg/git/gogit/checkout.go b/pkg/git/gogit/checkout.go index 37c958f4..c3c484c6 100644 --- a/pkg/git/gogit/checkout.go +++ b/pkg/git/gogit/checkout.go @@ -22,6 +22,7 @@ import ( "fmt" "io" "sort" + "strings" "time" "github.com/Masterminds/semver/v3" @@ -78,10 +79,21 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g } if currentRevision != "" && currentRevision == c.LastRevision { - return nil, git.NoChangesError{ - Message: "no changes since last reconciliation", - ObservedRevision: currentRevision, + // Construct a partial commit with the existing information. + // Split the revision and take the last part as the hash. + // Example revision: main/43d7eb9c49cdd49b2494efd481aea1166fc22b67 + var hash git.Hash + ss := strings.Split(currentRevision, "/") + if len(ss) > 1 { + hash = git.Hash(ss[len(ss)-1]) + } else { + hash = git.Hash(ss[0]) } + c := &git.Commit{ + Hash: hash, + Reference: plumbing.NewBranchReferenceName(c.Branch).String(), + } + return c, nil } } @@ -153,10 +165,21 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git. } if currentRevision != "" && currentRevision == c.LastRevision { - return nil, git.NoChangesError{ - Message: "no changes since last reconciliation", - ObservedRevision: currentRevision, + // Construct a partial commit with the existing information. + // Split the revision and take the last part as the hash. + // Example revision: 6.1.4/bf09377bfd5d3bcac1e895fa8ce52dc76695c060 + var hash git.Hash + ss := strings.Split(currentRevision, "/") + if len(ss) > 1 { + hash = git.Hash(ss[len(ss)-1]) + } else { + hash = git.Hash(ss[0]) } + c := &git.Commit{ + Hash: hash, + Reference: ref.String(), + } + return c, nil } } repo, err := extgogit.PlainCloneContext(ctx, path, false, &extgogit.CloneOptions{ diff --git a/pkg/git/gogit/checkout_test.go b/pkg/git/gogit/checkout_test.go index ed69f670..b5b6b90b 100644 --- a/pkg/git/gogit/checkout_test.go +++ b/pkg/git/gogit/checkout_test.go @@ -67,32 +67,36 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } tests := []struct { - name string - branch string - filesCreated map[string]string - expectedCommit string - expectedErr string - lastRevision string + name string + branch string + filesCreated map[string]string + lastRevision string + expectedCommit string + expectedConcreteCommit bool + expectedErr string }{ { - name: "Default branch", - branch: "master", - filesCreated: map[string]string{"branch": "init"}, - expectedCommit: firstCommit.String(), + name: "Default branch", + branch: "master", + filesCreated: map[string]string{"branch": "init"}, + expectedCommit: firstCommit.String(), + expectedConcreteCommit: true, }, { - name: "skip clone if LastRevision hasn't changed", - branch: "master", - filesCreated: map[string]string{"branch": "init"}, - expectedErr: fmt.Sprintf("no changes since last reconciliation: observed revision 'master/%s'", firstCommit.String()), - lastRevision: fmt.Sprintf("master/%s", firstCommit.String()), + name: "skip clone if LastRevision hasn't changed", + branch: "master", + filesCreated: map[string]string{"branch": "init"}, + lastRevision: fmt.Sprintf("master/%s", firstCommit.String()), + expectedCommit: firstCommit.String(), + expectedConcreteCommit: false, }, { - name: "Other branch - revision has changed", - branch: "test", - filesCreated: map[string]string{"branch": "second"}, - expectedCommit: secondCommit.String(), - lastRevision: fmt.Sprintf("master/%s", firstCommit.String()), + name: "Other branch - revision has changed", + branch: "test", + filesCreated: map[string]string{"branch": "second"}, + lastRevision: fmt.Sprintf("master/%s", firstCommit.String()), + expectedCommit: secondCommit.String(), + expectedConcreteCommit: true, }, { name: "Non existing branch", @@ -120,59 +124,65 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } g.Expect(err).ToNot(HaveOccurred()) g.Expect(cc.String()).To(Equal(tt.branch + "/" + tt.expectedCommit)) + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectedConcreteCommit)) - for k, v := range tt.filesCreated { - g.Expect(filepath.Join(tmpDir, k)).To(BeARegularFile()) - g.Expect(os.ReadFile(filepath.Join(tmpDir, k))).To(BeEquivalentTo(v)) + if tt.expectedConcreteCommit { + for k, v := range tt.filesCreated { + g.Expect(filepath.Join(tmpDir, k)).To(BeARegularFile()) + g.Expect(os.ReadFile(filepath.Join(tmpDir, k))).To(BeEquivalentTo(v)) + } } }) } } func TestCheckoutTag_Checkout(t *testing.T) { + type testTag struct { + name string + annotated bool + } + tests := []struct { - name string - tag string - annotated bool - checkoutTag string - expectTag string - expectErr string - lastRev string - setLastRev bool + name string + tagsInRepo []testTag + checkoutTag string + lastRevTag string + expectConcreteCommit bool + expectErr string }{ { - name: "Tag", - tag: "tag-1", - checkoutTag: "tag-1", - expectTag: "tag-1", + name: "Tag", + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "tag-1", + expectConcreteCommit: true, }, { - name: "Skip Tag if last revision hasn't changed", - tag: "tag-2", - checkoutTag: "tag-2", - setLastRev: true, - expectErr: "no changes since last reconciliation", + name: "Annotated", + tagsInRepo: []testTag{{"annotated", true}}, + checkoutTag: "annotated", + expectConcreteCommit: true, }, { - name: "Last revision changed", - tag: "tag-3", - checkoutTag: "tag-3", - expectTag: "tag-3", - lastRev: "tag-3/", - }, - { - name: "Annotated", - tag: "annotated", - annotated: true, - checkoutTag: "annotated", - expectTag: "annotated", - }, - { - name: "Non existing tag", - tag: "tag-1", + name: "Non existing tag", + // Without this go-git returns error "remote repository is empty". + tagsInRepo: []testTag{{"tag-1", false}}, checkoutTag: "invalid", expectErr: "couldn't find remote ref \"refs/tags/invalid\"", }, + { + name: "Skip clone - last revision unchanged", + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "tag-1", + lastRevTag: "tag-1", + expectConcreteCommit: false, + }, + { + name: "Last revision changed", + tagsInRepo: []testTag{{"tag-1", false}, {"tag-2", false}}, + checkoutTag: "tag-2", + lastRevTag: "tag-1", + expectConcreteCommit: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -183,32 +193,37 @@ func TestCheckoutTag_Checkout(t *testing.T) { t.Fatal(err) } - var h plumbing.Hash - var tagHash *plumbing.Reference - if tt.tag != "" { - h, err = commitFile(repo, "tag", tt.tag, time.Now()) - if err != nil { - t.Fatal(err) - } - tagHash, err = tag(repo, h, !tt.annotated, tt.tag, time.Now()) - if err != nil { - t.Fatal(err) + // Collect tags and their associated commit hash for later + // reference. + tagCommits := map[string]string{} + + // Populate the repo with commits and tags. + if tt.tagsInRepo != nil { + for _, tr := range tt.tagsInRepo { + h, err := commitFile(repo, "tag", tr.name, time.Now()) + if err != nil { + t.Fatal(err) + } + _, err = tag(repo, h, tr.annotated, tr.name, time.Now()) + if err != nil { + t.Fatal(err) + } + tagCommits[tr.name] = h.String() } } - tag := CheckoutTag{ + checkoutTag := CheckoutTag{ Tag: tt.checkoutTag, } - if tt.setLastRev { - tag.LastRevision = fmt.Sprintf("%s/%s", tt.tag, tagHash.Hash().String()) + // If last revision is provided, configure it. + if tt.lastRevTag != "" { + lc := tagCommits[tt.lastRevTag] + checkoutTag.LastRevision = fmt.Sprintf("%s/%s", tt.lastRevTag, lc) } - if tt.lastRev != "" { - tag.LastRevision = tt.lastRev - } tmpDir := t.TempDir() - cc, err := tag.Checkout(context.TODO(), tmpDir, path, nil) + cc, err := checkoutTag.Checkout(context.TODO(), tmpDir, path, nil) if tt.expectErr != "" { g.Expect(err).ToNot(BeNil()) g.Expect(err.Error()).To(ContainSubstring(tt.expectErr)) @@ -216,10 +231,17 @@ func TestCheckoutTag_Checkout(t *testing.T) { return } + // Check successful checkout results. + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectConcreteCommit)) + targetTagHash := tagCommits[tt.checkoutTag] g.Expect(err).ToNot(HaveOccurred()) - g.Expect(cc.String()).To(Equal(tt.expectTag + "/" + h.String())) - g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) - g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.tag)) + g.Expect(cc.String()).To(Equal(tt.checkoutTag + "/" + targetTagHash)) + + // Check file content only when there's an actual checkout. + if tt.lastRevTag != tt.checkoutTag { + g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) + g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.checkoutTag)) + } }) } } diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index 9dc233fe..b0912276 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -81,12 +81,15 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g return nil, fmt.Errorf("unable to remote ls for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) } if len(heads) > 0 { - currentRevision := fmt.Sprintf("%s/%s", c.Branch, heads[0].Id.String()) + hash := heads[0].Id.String() + currentRevision := fmt.Sprintf("%s/%s", c.Branch, hash) if currentRevision == c.LastRevision { - return nil, git.NoChangesError{ - Message: "no changes since last reconciliation", - ObservedRevision: currentRevision, + // Construct a partial commit with the existing information. + c := &git.Commit{ + Hash: git.Hash(hash), + Reference: "refs/heads/" + c.Branch, } + return c, nil } } } @@ -163,21 +166,25 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git. return nil, fmt.Errorf("unable to remote ls for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) } if len(heads) > 0 { - currentRevision := fmt.Sprintf("%s/%s", c.Tag, heads[0].Id.String()) + hash := heads[0].Id.String() + currentRevision := fmt.Sprintf("%s/%s", c.Tag, hash) var same bool if currentRevision == c.LastRevision { same = true } else if len(heads) > 1 { - currentAnnotatedRevision := fmt.Sprintf("%s/%s", c.Tag, heads[1].Id.String()) + hash = heads[1].Id.String() + currentAnnotatedRevision := fmt.Sprintf("%s/%s", c.Tag, hash) if currentAnnotatedRevision == c.LastRevision { same = true } } if same { - return nil, git.NoChangesError{ - Message: "no changes since last reconciliation", - ObservedRevision: currentRevision, + // Construct a partial commit with the existing information. + c := &git.Commit{ + Hash: git.Hash(hash), + Reference: "refs/tags/" + c.Tag, } + return c, nil } } } diff --git a/pkg/git/libgit2/checkout_test.go b/pkg/git/libgit2/checkout_test.go index 98b57b24..870c1d17 100644 --- a/pkg/git/libgit2/checkout_test.go +++ b/pkg/git/libgit2/checkout_test.go @@ -83,44 +83,49 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } tests := []struct { - name string - branch string - filesCreated map[string]string - expectedCommit string - expectedErr string - lastRevision string + name string + branch string + filesCreated map[string]string + lastRevision string + expectedCommit string + expectedConcreteCommit bool + expectedErr string }{ { - name: "Default branch", - branch: defaultBranch, - filesCreated: map[string]string{"branch": "second"}, - expectedCommit: secondCommit.String(), + name: "Default branch", + branch: defaultBranch, + filesCreated: map[string]string{"branch": "second"}, + expectedCommit: secondCommit.String(), + expectedConcreteCommit: true, }, { - name: "Other branch", - branch: "test", - filesCreated: map[string]string{"branch": "init"}, - expectedCommit: firstCommit.String(), + name: "Other branch", + branch: "test", + filesCreated: map[string]string{"branch": "init"}, + expectedCommit: firstCommit.String(), + expectedConcreteCommit: true, }, { - name: "Non existing branch", - branch: "invalid", - expectedErr: "reference 'refs/remotes/origin/invalid' not found", + name: "Non existing branch", + branch: "invalid", + expectedErr: "reference 'refs/remotes/origin/invalid' not found", + expectedConcreteCommit: true, }, { - name: "skip clone - lastRevision hasn't changed", - branch: defaultBranch, - filesCreated: map[string]string{"branch": "second"}, - expectedCommit: secondCommit.String(), - lastRevision: fmt.Sprintf("%s/%s", defaultBranch, secondCommit.String()), - expectedErr: fmt.Sprintf("no changes since last reconciliation: observed revision '%s/%s'", defaultBranch, secondCommit.String()), + name: "skip clone - lastRevision hasn't changed", + branch: defaultBranch, + filesCreated: map[string]string{"branch": "second"}, + lastRevision: fmt.Sprintf("%s/%s", defaultBranch, secondCommit.String()), + expectedCommit: secondCommit.String(), + expectedConcreteCommit: false, }, { - name: "lastRevision is different", - branch: defaultBranch, - filesCreated: map[string]string{"branch": "second"}, - expectedCommit: secondCommit.String(), - lastRevision: fmt.Sprintf("%s/%s", defaultBranch, firstCommit.String()), + name: "lastRevision is different", + branch: defaultBranch, + filesCreated: map[string]string{"branch": "second"}, + lastRevision: fmt.Sprintf("%s/%s", defaultBranch, firstCommit.String()), + expectedCommit: secondCommit.String(), + expectedConcreteCommit: true, }, } @@ -143,37 +148,43 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } g.Expect(err).ToNot(HaveOccurred()) g.Expect(cc.String()).To(Equal(tt.branch + "/" + tt.expectedCommit)) + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectedConcreteCommit)) - for k, v := range tt.filesCreated { - g.Expect(filepath.Join(tmpDir, k)).To(BeARegularFile()) - g.Expect(os.ReadFile(filepath.Join(tmpDir, k))).To(BeEquivalentTo(v)) + if tt.expectedConcreteCommit { + for k, v := range tt.filesCreated { + g.Expect(filepath.Join(tmpDir, k)).To(BeARegularFile()) + g.Expect(os.ReadFile(filepath.Join(tmpDir, k))).To(BeEquivalentTo(v)) + } } }) } } func TestCheckoutTag_Checkout(t *testing.T) { + type testTag struct { + name string + annotated bool + } + tests := []struct { - name string - tag string - annotated bool - checkoutTag string - expectTag string - expectErr string - lastRevision bool + name string + tagsInRepo []testTag + checkoutTag string + lastRevTag string + expectErr string + expectConcreteCommit bool }{ { - name: "Tag", - tag: "tag-1", - checkoutTag: "tag-1", - expectTag: "tag-1", + name: "Tag", + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "tag-1", + expectConcreteCommit: true, }, { - name: "Annotated", - tag: "annotated", - annotated: true, - checkoutTag: "annotated", - expectTag: "annotated", + name: "Annotated", + tagsInRepo: []testTag{{"annotated", true}}, + checkoutTag: "annotated", + expectConcreteCommit: true, }, { name: "Non existing tag", @@ -181,19 +192,18 @@ func TestCheckoutTag_Checkout(t *testing.T) { expectErr: "unable to find 'invalid': no reference found for shorthand 'invalid'", }, { - name: "skip clone - last revision is unchanged", - tag: "tag-1", - checkoutTag: "tag-1", - expectTag: "tag-1", - lastRevision: true, - expectErr: "no changes since last reconciliation", + name: "Skip clone - last revision unchanged", + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "tag-1", + lastRevTag: "tag-1", + expectConcreteCommit: false, }, { - name: "last revision changed", - tag: "tag-1", - checkoutTag: "tag-1", - expectTag: "tag-2", - lastRevision: true, + name: "Last revision changed", + tagsInRepo: []testTag{{"tag-1", false}, {"tag-2", false}}, + checkoutTag: "tag-2", + lastRevTag: "tag-1", + expectConcreteCommit: true, }, } for _, tt := range tests { @@ -206,68 +216,57 @@ func TestCheckoutTag_Checkout(t *testing.T) { } defer repo.Free() - var commit *git2go.Commit - if tt.tag != "" { - c, err := commitFile(repo, "tag", tt.tag, time.Now()) - if err != nil { - t.Fatal(err) - } - if commit, err = repo.LookupCommit(c); err != nil { - t.Fatal(err) - } - _, err = tag(repo, commit.Id(), !tt.annotated, tt.tag, time.Now()) - if err != nil { - t.Fatal(err) - } - } + // Collect tags and their associated commit for later reference. + tagCommits := map[string]*git2go.Commit{} - checkoutTag := CheckoutTag{ - Tag: tt.checkoutTag, - } - tmpDir := t.TempDir() - - cc, err := checkoutTag.Checkout(context.TODO(), tmpDir, repo.Path(), nil) - - if tt.expectErr != "" { - if tt.lastRevision { - tmpDir, _ = os.MkdirTemp("", "test") - defer os.RemoveAll(tmpDir) - checkoutTag.LastRevision = cc.String() - cc, err = checkoutTag.Checkout(context.TODO(), tmpDir, repo.Path(), nil) - } - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.expectErr)) - g.Expect(cc).To(BeNil()) - return - } - if tt.lastRevision { - checkoutTag.LastRevision = fmt.Sprintf("%s/%s", tt.tag, commit.Id().String()) - checkoutTag.Tag = tt.expectTag - if tt.tag != "" { - c, err := commitFile(repo, "tag", "changed tag", time.Now()) + // Populate the repo with commits and tags. + if tt.tagsInRepo != nil { + for _, tr := range tt.tagsInRepo { + var commit *git2go.Commit + c, err := commitFile(repo, "tag", tr.name, time.Now()) if err != nil { t.Fatal(err) } if commit, err = repo.LookupCommit(c); err != nil { t.Fatal(err) } - _, err = tag(repo, commit.Id(), !tt.annotated, tt.expectTag, time.Now()) + _, err = tag(repo, commit.Id(), tr.annotated, tr.name, time.Now()) if err != nil { t.Fatal(err) } - tmpDir, _ = os.MkdirTemp("", "test") - defer os.RemoveAll(tmpDir) - cc, err = checkoutTag.Checkout(context.TODO(), tmpDir, repo.Path(), nil) + tagCommits[tr.name] = commit } } + checkoutTag := CheckoutTag{ + Tag: tt.checkoutTag, + } + // If last revision is provided, configure it. + if tt.lastRevTag != "" { + lc := tagCommits[tt.lastRevTag] + checkoutTag.LastRevision = fmt.Sprintf("%s/%s", tt.lastRevTag, lc.Id().String()) + } + + tmpDir := t.TempDir() + + cc, err := checkoutTag.Checkout(context.TODO(), tmpDir, repo.Path(), nil) + if tt.expectErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.expectErr)) + g.Expect(cc).To(BeNil()) + return + } + + // Check successful checkout results. + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectConcreteCommit)) + targetTagCommit := tagCommits[tt.checkoutTag] g.Expect(err).ToNot(HaveOccurred()) - g.Expect(cc.String()).To(Equal(tt.expectTag + "/" + commit.Id().String())) - g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) - if tt.lastRevision { - g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo("changed tag")) - } else { - g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.tag)) + g.Expect(cc.String()).To(Equal(tt.checkoutTag + "/" + targetTagCommit.Id().String())) + + // Check file content only when there's an actual checkout. + if tt.lastRevTag != tt.checkoutTag { + g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) + g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.checkoutTag)) } }) } diff --git a/pkg/git/options.go b/pkg/git/options.go index b5e8f2c4..ff1bccac 100644 --- a/pkg/git/options.go +++ b/pkg/git/options.go @@ -49,8 +49,7 @@ type CheckoutOptions struct { // not supported by all Implementations. RecurseSubmodules bool - // LastRevision holds the revision observed on the last successful - // reconciliation. + // LastRevision holds the last observed revision of the local repository. // It is used to skip clone operations when no changes were detected. LastRevision string }