From 7a0221bf98dae087c6a73e2e1d14a88a02fb7cc3 Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 25 Feb 2022 17:03:04 +0530 Subject: [PATCH] Remove redundant reconciling in reconcileArtifact reconcileSource() adds reconciling condition with accurate information. Remove setting reconciling condition in reconcileArtifact(). Signed-off-by: Sunny --- controllers/bucket_controller.go | 4 ---- controllers/bucket_controller_test.go | 9 --------- controllers/gitrepository_controller.go | 9 +++------ controllers/gitrepository_controller_test.go | 12 +----------- controllers/helmrepository_controller.go | 4 ---- controllers/helmrepository_controller_test.go | 3 --- 6 files changed, 4 insertions(+), 37 deletions(-) diff --git a/controllers/bucket_controller.go b/controllers/bucket_controller.go index 522b47c81..324cf46e0 100644 --- a/controllers/bucket_controller.go +++ b/controllers/bucket_controller.go @@ -626,10 +626,6 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, return sreconcile.ResultSuccess, nil } - // Mark reconciling because the artifact and remote source are different. - // and they have to be reconciled. - conditions.MarkReconciling(obj, "NewRevision", "new upstream revision '%s'", artifact.Revision) - // Ensure target path exists and is a directory if f, err := os.Stat(dir); err != nil { return sreconcile.ResultEmpty, &serror.Event{ diff --git a/controllers/bucket_controller_test.go b/controllers/bucket_controller_test.go index 39ef70860..17429fb92 100644 --- a/controllers/bucket_controller_test.go +++ b/controllers/bucket_controller_test.go @@ -870,7 +870,6 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'existing'"), }, }, { @@ -896,7 +895,6 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'existing'"), }, }, { @@ -914,7 +912,6 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'existing'"), }, }, { @@ -924,9 +921,6 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultEmpty, wantErr: true, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'existing'"), - }, }, { name: "Dir path is not a directory", @@ -943,9 +937,6 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultEmpty, wantErr: true, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'existing'"), - }, }, } diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 53a9da69e..f53a835d5 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -405,10 +405,6 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, return sreconcile.ResultSuccess, nil } - // Mark reconciling because the artifact and remote source are different. - // and they have to be reconciled. - conditions.MarkReconciling(obj, "NewRevision", "new upstream revision '%s'", artifact.Revision) - // Ensure target path exists and is a directory if f, err := os.Stat(dir); err != nil { e := &serror.Event{ @@ -540,8 +536,9 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, // Observe if the artifacts still match the previous included ones if artifacts.Diff(obj.Status.IncludedArtifacts) { - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", - "included artifacts differ from last observed includes") + message := fmt.Sprintf("included artifacts differ from last observed includes") + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", message) + conditions.MarkReconciling(obj, "IncludeChange", message) } // Persist the artifactSet. diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index 1e7028c75..a91a0f624 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -717,7 +717,6 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'main/revision'"), }, }, { @@ -736,7 +735,6 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'main/revision'"), }, }, { @@ -770,7 +768,6 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'main/revision'"), }, }, { @@ -788,7 +785,6 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'main/revision'"), }, }, { @@ -809,24 +805,17 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'main/revision'"), }, }, { name: "Target path does not exists", dir: "testdata/git/foo", wantErr: true, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'main/revision'"), - }, }, { name: "Target path is not a directory", dir: "testdata/git/repository/foo.txt", wantErr: true, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new upstream revision 'main/revision'"), - }, }, } @@ -926,6 +915,7 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "IncludeChange", "included artifacts differ from last observed includes"), + *conditions.TrueCondition(meta.ReconcilingCondition, "IncludeChange", "included artifacts differ from last observed includes"), }, }, { diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index bfdce2958..bc154da73 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -396,10 +396,6 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *s return sreconcile.ResultSuccess, nil } - // Mark reconciling because the artifact and remote source are different. - // and they have to be reconciled. - conditions.MarkReconciling(obj, "NewRevision", "new index revision '%s'", artifact.Revision) - // Create artifact dir if err := r.Storage.MkdirAll(*artifact); err != nil { return sreconcile.ResultEmpty, &serror.Event{ diff --git a/controllers/helmrepository_controller_test.go b/controllers/helmrepository_controller_test.go index 137df58f8..a337d04bb 100644 --- a/controllers/helmrepository_controller_test.go +++ b/controllers/helmrepository_controller_test.go @@ -520,7 +520,6 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision 'existing'"), }, }, { @@ -546,7 +545,6 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision 'existing'"), }, }, { @@ -564,7 +562,6 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision 'existing'"), }, }, }