gitrepo: Use commit msg in NewArtifact message

Use commit message in the NewArtifact event message to make it more user
friendly.

Signed-off-by: Sunny <[email protected]>

Author: darkowlzz

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

controllers/gitrepository_controller_test.go

@@ -511,14 +511,14 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {
 						assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "<url>", 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,17 +666,17 @@ 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())
 					}
 					g.Expect(err != nil).To(Equal(tt.wantErr))
 					g.Expect(got).To(Equal(tt.want))
 					if tt.wantRevision != "" {
 						revision := strings.ReplaceAll(tt.wantRevision, "<commit>", 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 {