diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 8c02f117..8c4c31b9 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -107,7 +107,7 @@ type GitRepositoryReconcilerOptions struct { // gitRepoReconcilerFunc is the function type for all the Git repository // reconciler functions. -type gitRepoReconcilerFunc func(ctx context.Context, obj *sourcev1.GitRepository, artifact *sourcev1.Artifact, includes *artifactSet, dir string) (sreconcile.Result, error) +type gitRepoReconcilerFunc func(ctx context.Context, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) func (r *GitRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error { return r.SetupWithManagerAndOptions(mgr, GitRepositoryReconcilerOptions{}) @@ -207,7 +207,7 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.G conditions.MarkReconciling(obj, "NewGeneration", "reconciling new object generation (%d)", obj.Generation) } - var artifact sourcev1.Artifact + var commit git.Commit var includes artifactSet // Create temp dir for Git clone @@ -224,7 +224,7 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.G var res sreconcile.Result var resErr error for _, rec := range reconcilers { - recResult, err := rec(ctx, obj, &artifact, &includes, tmpDir) + recResult, err := rec(ctx, obj, &commit, &includes, tmpDir) // Exit immediately on ResultRequeue. if recResult == sreconcile.ResultRequeue { return sreconcile.ResultRequeue, nil @@ -248,7 +248,8 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.G // If the artifact in the Status object of the resource disappeared from storage, it is removed from the object. // If the object does not have an artifact in its Status object, a v1beta1.ArtifactUnavailableCondition is set. // If the hostname of any of the URLs on the object do not match the current storage server hostname, they are updated. -func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.GitRepository, artifact *sourcev1.Artifact, includes *artifactSet, dir string) (sreconcile.Result, error) { +func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, + obj *sourcev1.GitRepository, _ *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) @@ -284,7 +285,7 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sou // If both the checkout and signature verification are successful, the given artifact pointer is set to a new artifact // with the available metadata. func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, - obj *sourcev1.GitRepository, artifact *sourcev1.Artifact, _ *artifactSet, dir string) (sreconcile.Result, error) { + obj *sourcev1.GitRepository, commit *git.Commit, _ *artifactSet, dir string) (sreconcile.Result, error) { // Configure authentication strategy to access the source var authOpts *git.AuthOptions var err error @@ -344,7 +345,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // Checkout HEAD of reference in object gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) defer cancel() - commit, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts) + c, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to checkout and determine revision: %w", err), @@ -354,6 +355,8 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // Coin flip on transient or persistent error, return error and hope for the best return sreconcile.ResultEmpty, e } + // Assign the commit to the shared commit reference. + *commit = *c ctrl.LoggerFrom(ctx).V(logger.DebugLevel).Info("git repository checked out", "url", obj.Spec.URL, "revision", commit.String()) conditions.Delete(obj, sourcev1.FetchFailedCondition) @@ -362,9 +365,6 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, return result, err } - // Create potential new artifact with current available metadata - *artifact = r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), commit.String(), fmt.Sprintf("%s.tar.gz", commit.Hash.String())) - // Mark observations about the revision on the object if !obj.GetArtifact().HasRevision(commit.String()) { message := fmt.Sprintf("new upstream revision '%s'", commit.String()) @@ -383,7 +383,11 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // Source ignore patterns are loaded, and the given directory is archived. // On a successful archive, the artifact and includes in the status of the given object are set, and the symlink in the // storage is updated to its path. -func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.GitRepository, artifact *sourcev1.Artifact, includes *artifactSet, dir string) (sreconcile.Result, error) { +func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, + obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { + // Create potential new artifact with current available metadata + artifact := r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), commit.String(), fmt.Sprintf("%s.tar.gz", commit.Hash.String())) + // Always restore the Ready condition in case it got removed due to a transient error defer func() { if obj.GetArtifact().HasRevision(artifact.Revision) && !includes.Diff(obj.Status.IncludedArtifacts) { @@ -419,14 +423,14 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *so } // Ensure artifact directory exists and acquire lock - if err := r.Storage.MkdirAll(*artifact); err != nil { + if err := r.Storage.MkdirAll(artifact); err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to create artifact directory: %w", err), Reason: sourcev1.StorageOperationFailedReason, } return sreconcile.ResultEmpty, e } - unlock, err := r.Storage.Lock(*artifact) + unlock, err := r.Storage.Lock(artifact) if err != nil { return sreconcile.ResultEmpty, &serror.Event{ Err: fmt.Errorf("failed to acquire lock for artifact: %w", err), @@ -448,7 +452,7 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *so } // Archive directory to storage - if err := r.Storage.Archive(artifact, dir, SourceIgnoreFilter(ps, nil)); err != nil { + if err := r.Storage.Archive(&artifact, dir, SourceIgnoreFilter(ps, nil)); err != nil { return sreconcile.ResultEmpty, &serror.Event{ Err: fmt.Errorf("unable to archive artifact to storage: %w", err), Reason: sourcev1.StorageOperationFailedReason, @@ -457,14 +461,14 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *so r.AnnotatedEventf(obj, map[string]string{ "revision": artifact.Revision, "checksum": artifact.Checksum, - }, corev1.EventTypeNormal, "NewArtifact", "stored artifact for revision '%s'", artifact.Revision) + }, corev1.EventTypeNormal, "NewArtifact", "stored artifact for commit '%s'", commit.ShortMessage()) // Record it on the object obj.Status.Artifact = artifact.DeepCopy() obj.Status.IncludedArtifacts = *includes // Update symlink on a "best effort" basis - url, err := r.Storage.Symlink(*artifact, "latest.tar.gz") + url, err := r.Storage.Symlink(artifact, "latest.tar.gz") if err != nil { r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason, "failed to update status URL symlink: %s", err) @@ -481,7 +485,8 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *so // If an include is unavailable, it marks the object with v1beta1.IncludeUnavailableCondition and returns early. // If the copy operations are successful, it deletes the v1beta1.IncludeUnavailableCondition from the object. // If the artifactSet differs from the current set, it marks the object with v1beta1.ArtifactOutdatedCondition. -func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, obj *sourcev1.GitRepository, _ *sourcev1.Artifact, includes *artifactSet, dir string) (sreconcile.Result, error) { +func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, + obj *sourcev1.GitRepository, _ *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { artifacts := make(artifactSet, len(obj.Spec.Include)) for i, incl := range obj.Spec.Include { // Do this first as it is much cheaper than copy operations diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index 9473e8dd..5f852977 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -511,14 +511,14 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", obj.Spec.URL) } - var artifact sourcev1.Artifact + var commit git.Commit var includes artifactSet - got, err := r.reconcileSource(context.TODO(), obj, &artifact, &includes, tmpDir) + got, err := r.reconcileSource(context.TODO(), obj, &commit, &includes, tmpDir) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) - g.Expect(artifact).ToNot(BeNil()) + g.Expect(commit).ToNot(BeNil()) }) } }) @@ -666,9 +666,9 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) obj := obj.DeepCopy() obj.Spec.GitImplementation = i - var artifact sourcev1.Artifact + var commit git.Commit var includes artifactSet - got, err := r.reconcileSource(ctx, obj, &artifact, &includes, tmpDir) + got, err := r.reconcileSource(ctx, obj, &commit, &includes, tmpDir) if err != nil { println(err.Error()) } @@ -676,7 +676,7 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) g.Expect(got).To(Equal(tt.want)) if tt.wantRevision != "" { revision := strings.ReplaceAll(tt.wantRevision, "", headRef.Hash().String()) - g.Expect(artifact.Revision).To(Equal(revision)) + g.Expect(commit.String()).To(Equal(revision)) g.Expect(conditions.IsTrue(obj, sourcev1.ArtifactOutdatedCondition)).To(BeTrue()) } }) @@ -691,7 +691,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { dir string includes artifactSet beforeFunc func(obj *sourcev1.GitRepository) - afterFunc func(t *WithT, obj *sourcev1.GitRepository, artifact sourcev1.Artifact) + afterFunc func(t *WithT, obj *sourcev1.GitRepository) want sreconcile.Result wantErr bool assertConditions []metav1.Condition @@ -702,7 +702,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { beforeFunc: func(obj *sourcev1.GitRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} }, - afterFunc: func(t *WithT, obj *sourcev1.GitRepository, artifact sourcev1.Artifact) { + afterFunc: func(t *WithT, obj *sourcev1.GitRepository) { t.Expect(obj.GetArtifact()).ToNot(BeNil()) t.Expect(obj.Status.URL).ToNot(BeEmpty()) }, @@ -719,7 +719,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { beforeFunc: func(obj *sourcev1.GitRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} }, - afterFunc: func(t *WithT, obj *sourcev1.GitRepository, artifact sourcev1.Artifact) { + afterFunc: func(t *WithT, obj *sourcev1.GitRepository) { t.Expect(obj.GetArtifact()).ToNot(BeNil()) t.Expect(obj.GetArtifact().Checksum).To(Equal("60a3bf69f337cb5ec9ebd00abefbb6e7f2a2cf27158ecf438d52b2035b184172")) t.Expect(obj.Status.IncludedArtifacts).ToNot(BeEmpty()) @@ -740,7 +740,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { obj.Status.Artifact = &sourcev1.Artifact{Revision: "main/revision"} obj.Status.IncludedArtifacts = []*sourcev1.Artifact{{Revision: "main/revision"}} }, - afterFunc: func(t *WithT, obj *sourcev1.GitRepository, artifact sourcev1.Artifact) { + afterFunc: func(t *WithT, obj *sourcev1.GitRepository) { t.Expect(obj.Status.URL).To(BeEmpty()) }, want: sreconcile.ResultSuccess, @@ -755,7 +755,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { obj.Spec.Interval = metav1.Duration{Duration: interval} obj.Spec.Ignore = pointer.StringPtr("!**.txt\n") }, - afterFunc: func(t *WithT, obj *sourcev1.GitRepository, artifact sourcev1.Artifact) { + afterFunc: func(t *WithT, obj *sourcev1.GitRepository) { t.Expect(obj.GetArtifact()).ToNot(BeNil()) t.Expect(obj.GetArtifact().Checksum).To(Equal("11f7f007dce5619bd79e6c57688261058d09f5271e802463ac39f2b9ead7cabd")) }, @@ -772,7 +772,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { obj.Spec.Interval = metav1.Duration{Duration: interval} conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "") }, - afterFunc: func(t *WithT, obj *sourcev1.GitRepository, artifact sourcev1.Artifact) { + afterFunc: func(t *WithT, obj *sourcev1.GitRepository) { t.Expect(obj.GetArtifact()).ToNot(BeNil()) t.Expect(obj.GetArtifact().Checksum).To(Equal("60a3bf69f337cb5ec9ebd00abefbb6e7f2a2cf27158ecf438d52b2035b184172")) t.Expect(obj.Status.URL).ToNot(BeEmpty()) @@ -789,7 +789,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { beforeFunc: func(obj *sourcev1.GitRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} }, - afterFunc: func(t *WithT, obj *sourcev1.GitRepository, artifact sourcev1.Artifact) { + afterFunc: func(t *WithT, obj *sourcev1.GitRepository) { t.Expect(obj.GetArtifact()).ToNot(BeNil()) localPath := testStorage.LocalPath(*obj.GetArtifact()) @@ -843,15 +843,18 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { tt.beforeFunc(obj) } - artifact := testStorage.NewArtifactFor(obj.Kind, obj, "main/revision", "checksum.tar.gz") + commit := git.Commit{ + Hash: []byte("revision"), + Reference: "refs/heads/main", + } - got, err := r.reconcileArtifact(ctx, obj, &artifact, &tt.includes, tt.dir) + got, err := r.reconcileArtifact(ctx, obj, &commit, &tt.includes, tt.dir) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) if tt.afterFunc != nil { - tt.afterFunc(g, obj, artifact) + tt.afterFunc(g, obj) } }) } @@ -1036,10 +1039,10 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) defer os.RemoveAll(tmpDir) - var artifact sourcev1.Artifact + var commit git.Commit var includes artifactSet - got, err := r.reconcileInclude(ctx, obj, &artifact, &includes, tmpDir) + got, err := r.reconcileInclude(ctx, obj, &commit, &includes, tmpDir) g.Expect(obj.GetConditions()).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) if err == nil {