diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index f7df88e0..54ba3dab 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -291,11 +291,30 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.G return res, resErr } -// notify emits notification related to the reconciliation. -func (r *GitRepositoryReconciler) notify(oldObj, newObj *sourcev1.GitRepository, commit git.Commit, res sreconcile.Result, resErr error) { - // Notify successful reconciliation for new artifact and recovery from any - // failure. +// shouldNotify analyzes the result of subreconcilers and determines if a +// notification should be sent. It decides about the final informational +// notifications after the reconciliation. Failure notification and in-line +// notifications are not handled here. +func (r *GitRepositoryReconciler) shouldNotify(oldObj, newObj *sourcev1.GitRepository, res sreconcile.Result, resErr error) bool { + // Notify for successful reconciliation. if resErr == nil && res == sreconcile.ResultSuccess && newObj.Status.Artifact != nil { + return true + } + // Notify for no-op reconciliation with ignore error. + if resErr != nil && res == sreconcile.ResultEmpty && newObj.Status.Artifact != nil { + // Convert to Generic error and check for ignore. + if ge, ok := resErr.(*serror.Generic); ok { + return ge.Ignore == true + } + } + return false +} + +// notify emits notification related to the result of reconciliation. +func (r *GitRepositoryReconciler) notify(oldObj, newObj *sourcev1.GitRepository, commit git.Commit, res sreconcile.Result, resErr error) { + // Notify successful reconciliation for new artifact, no-op reconciliation + // and recovery from any failure. + if r.shouldNotify(oldObj, newObj, res, resErr) { annotations := map[string]string{ sourcev1.GroupVersion.Group + "/revision": newObj.Status.Artifact.Revision, sourcev1.GroupVersion.Group + "/checksum": newObj.Status.Artifact.Checksum, @@ -306,7 +325,14 @@ func (r *GitRepositoryReconciler) notify(oldObj, newObj *sourcev1.GitRepository, oldChecksum = oldObj.GetArtifact().Checksum } - message := fmt.Sprintf("stored artifact for commit '%s'", commit.ShortMessage()) + // A partial commit due to no-op clone doesn't contain the commit + // message information. Have separate message for it. + var message string + if git.IsConcreteCommit(commit) { + message = fmt.Sprintf("stored artifact for commit '%s'", commit.ShortMessage()) + } else { + message = fmt.Sprintf("stored artifact for commit '%s'", commit.String()) + } // Notify on new artifact and failure recovery. if oldChecksum != newObj.GetArtifact().Checksum { diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index bc5749e4..3ace77f7 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -57,6 +57,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" + serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/features" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" @@ -1809,12 +1810,25 @@ func TestGitRepositoryReconciler_statusConditions(t *testing.T) { } func TestGitRepositoryReconciler_notify(t *testing.T) { + concreteCommit := git.Commit{ + Hash: git.Hash("some-hash"), + Message: "test commit", + Encoded: []byte("content"), + } + partialCommit := git.Commit{ + Hash: git.Hash("some-hash"), + } + + noopErr := serror.NewGeneric(fmt.Errorf("some no-op error"), "NoOpReason") + noopErr.Ignore = true + tests := []struct { name string res sreconcile.Result resErr error oldObjBeforeFunc func(obj *sourcev1.GitRepository) newObjBeforeFunc func(obj *sourcev1.GitRepository) + commit git.Commit wantEvent string }{ { @@ -1829,7 +1843,8 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { newObjBeforeFunc: func(obj *sourcev1.GitRepository) { obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"} }, - wantEvent: "Normal NewArtifact stored artifact for commit", + commit: concreteCommit, + wantEvent: "Normal NewArtifact stored artifact for commit 'test commit'", }, { name: "recovery from failure", @@ -1844,7 +1859,8 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"} conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") }, - wantEvent: "Normal Succeeded stored artifact for commit", + commit: concreteCommit, + wantEvent: "Normal Succeeded stored artifact for commit 'test commit'", }, { name: "recovery and new artifact", @@ -1859,7 +1875,8 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { obj.Status.Artifact = &sourcev1.Artifact{Revision: "aaa", Checksum: "bbb"} conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") }, - wantEvent: "Normal NewArtifact stored artifact for commit", + commit: concreteCommit, + wantEvent: "Normal NewArtifact stored artifact for commit 'test commit'", }, { name: "no updates", @@ -1874,6 +1891,22 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") }, }, + { + name: "no-op error result", + res: sreconcile.ResultEmpty, + resErr: noopErr, + oldObjBeforeFunc: func(obj *sourcev1.GitRepository) { + obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"} + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "fail") + conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, "foo") + }, + newObjBeforeFunc: func(obj *sourcev1.GitRepository) { + obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"} + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") + }, + commit: partialCommit, // no-op will always result in partial commit. + wantEvent: "Normal Succeeded stored artifact for commit 'HEAD/some-hash'", + }, } for _, tt := range tests { @@ -1895,10 +1928,7 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { EventRecorder: recorder, features: features.FeatureGates(), } - commit := &git.Commit{ - Message: "test commit", - } - reconciler.notify(oldObj, newObj, *commit, tt.res, tt.resErr) + reconciler.notify(oldObj, newObj, tt.commit, tt.res, tt.resErr) select { case x, ok := <-recorder.Events: