From a8bd0f84b6d5058f5cdffa3c369af1c1b34764ef Mon Sep 17 00:00:00 2001 From: Hidetake Iwata Date: Thu, 22 Sep 2022 10:00:20 +0900 Subject: [PATCH 1/7] Set deployment to inactive on terminating --- .github/workflows/docker.yaml | 21 ++++++++++++--------- controllers/applicationphase_controller.go | 9 +-------- e2e_test/Makefile | 5 ++++- e2e_test/applications/app1.yaml | 2 ++ e2e_test/applications/app2.yaml | 2 ++ e2e_test/applications/app3.yaml | 2 ++ pkg/notification/deployment.go | 3 +++ 7 files changed, 26 insertions(+), 18 deletions(-) diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index e2426e7b..ea49b224 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -99,13 +99,22 @@ jobs: GITHUB_APP_INSTALLATION_ID: ${{ secrets.E2E_GITHUB_APP_INSTALLATION_ID }} GITHUB_APP_PRIVATE_KEY: ${{ secrets.E2E_GITHUB_APP_PRIVATE_KEY }} - - run: make -C e2e_test wait-for-sync + - run: make -C e2e_test wait-for-apps # test - uses: int128/deployment-action@v1 id: deployment-app1 with: environment-suffix: /app1 + - uses: int128/deployment-action@v1 + id: deployment-app2 + with: + environment-suffix: /app2 + - uses: int128/deployment-action@v1 + id: deployment-app3 + with: + environment-suffix: /app3 + - run: make -C e2e_test deploy-app1 env: PULL_REQUEST_BODY: "e2e-test ${{ github.repository }}#${{ github.event.pull_request.number }}" @@ -114,26 +123,20 @@ jobs: - run: make -C e2e_test restart-app1 - - uses: int128/deployment-action@v1 - id: deployment-app2 - with: - environment-suffix: /app2 - run: make -C e2e_test deploy-app2 env: PULL_REQUEST_BODY: "e2e-test ${{ github.repository }}#${{ github.event.pull_request.number }}" DEPLOYMENT_URL: ${{ steps.deployment-app2.outputs.url }} GITHUB_TOKEN: ${{ steps.octoken.outputs.token }} - - uses: int128/deployment-action@v1 - id: deployment-app3 - with: - environment-suffix: /app3 - run: make -C e2e_test deploy-app3 env: PULL_REQUEST_BODY: "e2e-test ${{ github.repository }}#${{ github.event.pull_request.number }}" DEPLOYMENT_URL: ${{ steps.deployment-app3.outputs.url }} GITHUB_TOKEN: ${{ steps.octoken.outputs.token }} + - run: make -C e2e_test delete-apps + # show logs - run: make -C e2e_test logs-argocd if: always() diff --git a/controllers/applicationphase_controller.go b/controllers/applicationphase_controller.go index b4957205..faac4ced 100644 --- a/controllers/applicationphase_controller.go +++ b/controllers/applicationphase_controller.go @@ -20,7 +20,6 @@ import ( "context" argocdv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" - synccommon "github.com/argoproj/gitops-engine/pkg/sync/common" "github.com/int128/argocd-commenter/controllers/predicates" "github.com/int128/argocd-commenter/pkg/notification" "k8s.io/apimachinery/pkg/runtime" @@ -90,11 +89,5 @@ func (applicationPhaseComparer) Compare(applicationOld, applicationNew argocdv1a return false } } - - // notify only the following phases - switch applicationNew.Status.OperationState.Phase { - case synccommon.OperationRunning, synccommon.OperationSucceeded, synccommon.OperationFailed, synccommon.OperationError: - return true - } - return false + return true } diff --git a/e2e_test/Makefile b/e2e_test/Makefile index ad9450df..156aeecb 100644 --- a/e2e_test/Makefile +++ b/e2e_test/Makefile @@ -32,7 +32,7 @@ deploy-controller: cluster bash controller/create-controller-manager-secret.sh kubectl -n argocd-commenter-system rollout status deployment argocd-commenter-controller-manager -wait-for-sync: +wait-for-apps: go run ./waitforapp -revision "`git -C $(FIXTURE_DIR) rev-parse $(FIXTURE_BASE_BRANCH)`" app1 app2 app3 # fixture @@ -55,6 +55,9 @@ deploy-app3: $(MAKE) -C $(FIXTURE_DIR) update-manifest-app3 go run ./waitforapp -revision "`git -C $(FIXTURE_DIR) rev-parse $(FIXTURE_BASE_BRANCH)`" app3 +delete-apps: + kubectl -n argocd delete applications -l int128.github.io/e2e-test=fixture + logs-controller: -kubectl -n argocd-commenter-system logs -l control-plane=controller-manager --all-containers --tail=-1 logs-argocd: diff --git a/e2e_test/applications/app1.yaml b/e2e_test/applications/app1.yaml index 0ef2feff..943f655a 100644 --- a/e2e_test/applications/app1.yaml +++ b/e2e_test/applications/app1.yaml @@ -3,6 +3,8 @@ kind: Application metadata: name: app1 namespace: argocd + labels: + int128.github.io/e2e-test: fixture spec: project: default source: diff --git a/e2e_test/applications/app2.yaml b/e2e_test/applications/app2.yaml index ccf5b660..c0c2d0ff 100644 --- a/e2e_test/applications/app2.yaml +++ b/e2e_test/applications/app2.yaml @@ -3,6 +3,8 @@ kind: Application metadata: name: app2 namespace: argocd + labels: + int128.github.io/e2e-test: fixture spec: project: default source: diff --git a/e2e_test/applications/app3.yaml b/e2e_test/applications/app3.yaml index f7ac655d..a56f0e88 100644 --- a/e2e_test/applications/app3.yaml +++ b/e2e_test/applications/app3.yaml @@ -3,6 +3,8 @@ kind: Application metadata: name: app3 namespace: argocd + labels: + int128.github.io/e2e-test: fixture spec: project: default source: diff --git a/pkg/notification/deployment.go b/pkg/notification/deployment.go index 8f1e53c7..feab4a7c 100644 --- a/pkg/notification/deployment.go +++ b/pkg/notification/deployment.go @@ -66,6 +66,9 @@ func generateDeploymentStatus(e Event) *github.DeploymentStatus { case synccommon.OperationError: ds.State = "failure" return &ds + case synccommon.OperationTerminating: + ds.State = "inactive" + return &ds } } From 53ccbd95664239dbcdd599fec77e366d1d1e2679 Mon Sep 17 00:00:00 2001 From: Hidetake Iwata Date: Thu, 22 Sep 2022 10:17:45 +0900 Subject: [PATCH 2/7] Fix predicate --- controllers/applicationphase_controller.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/controllers/applicationphase_controller.go b/controllers/applicationphase_controller.go index faac4ced..9bae04a5 100644 --- a/controllers/applicationphase_controller.go +++ b/controllers/applicationphase_controller.go @@ -84,10 +84,12 @@ func (applicationPhaseComparer) Compare(applicationOld, applicationNew argocdv1a if applicationNew.Status.OperationState == nil { return false } - if applicationOld.Status.OperationState != nil { - if applicationOld.Status.OperationState.Phase == applicationNew.Status.OperationState.Phase { - return false - } + if applicationNew.Status.OperationState.Phase == "" { + return false + } + if applicationOld.Status.OperationState != nil && + applicationOld.Status.OperationState.Phase == applicationNew.Status.OperationState.Phase { + return false } return true } From 4dda2d2f797aa62532a7dc6aac289eccf76887b8 Mon Sep 17 00:00:00 2001 From: Hidetake Iwata Date: Thu, 22 Sep 2022 10:19:18 +0900 Subject: [PATCH 3/7] Add finalizer to applications --- e2e_test/applications/app1.yaml | 2 ++ e2e_test/applications/app2.yaml | 2 ++ e2e_test/applications/app3.yaml | 2 ++ 3 files changed, 6 insertions(+) diff --git a/e2e_test/applications/app1.yaml b/e2e_test/applications/app1.yaml index 943f655a..0ac34170 100644 --- a/e2e_test/applications/app1.yaml +++ b/e2e_test/applications/app1.yaml @@ -5,6 +5,8 @@ metadata: namespace: argocd labels: int128.github.io/e2e-test: fixture + finalizers: + - resources-finalizer.argocd.argoproj.io spec: project: default source: diff --git a/e2e_test/applications/app2.yaml b/e2e_test/applications/app2.yaml index c0c2d0ff..d1a01611 100644 --- a/e2e_test/applications/app2.yaml +++ b/e2e_test/applications/app2.yaml @@ -5,6 +5,8 @@ metadata: namespace: argocd labels: int128.github.io/e2e-test: fixture + finalizers: + - resources-finalizer.argocd.argoproj.io spec: project: default source: diff --git a/e2e_test/applications/app3.yaml b/e2e_test/applications/app3.yaml index a56f0e88..52e938e7 100644 --- a/e2e_test/applications/app3.yaml +++ b/e2e_test/applications/app3.yaml @@ -5,6 +5,8 @@ metadata: namespace: argocd labels: int128.github.io/e2e-test: fixture + finalizers: + - resources-finalizer.argocd.argoproj.io spec: project: default source: From 6f2d17ae7facd0ed33f85450fa3b9677dd41e56d Mon Sep 17 00:00:00 2001 From: Hidetake Iwata Date: Thu, 22 Sep 2022 11:19:34 +0900 Subject: [PATCH 4/7] Don't comment terminating status --- pkg/notification/comment.go | 49 +++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/pkg/notification/comment.go b/pkg/notification/comment.go index 32bb4307..876b2a79 100644 --- a/pkg/notification/comment.go +++ b/pkg/notification/comment.go @@ -85,25 +85,23 @@ func generateComment(e Event) string { return fmt.Sprintf(":warning: Syncing [%s](%s) to %s", e.Application.Name, argocdApplicationURL, revision) case synccommon.OperationSucceeded: return fmt.Sprintf(":white_check_mark: Synced [%s](%s) to %s", e.Application.Name, argocdApplicationURL, revision) + case synccommon.OperationFailed: + return fmt.Sprintf("## :x: Sync %s: [%s](%s)\nError while syncing to %s:\n%s", + e.Application.Status.OperationState.Phase, + e.Application.Name, + argocdApplicationURL, + revision, + generateSyncResultComment(e), + ) + case synccommon.OperationError: + return fmt.Sprintf("## :x: Sync %s: [%s](%s)\nError while syncing to %s:\n%s", + e.Application.Status.OperationState.Phase, + e.Application.Name, + argocdApplicationURL, + revision, + generateSyncResultComment(e), + ) } - - var b strings.Builder - b.WriteString(fmt.Sprintf("## :x: Sync %s: [%s](%s)\nError while syncing to %s:\n", - e.Application.Status.OperationState.Phase, - e.Application.Name, - argocdApplicationURL, - revision, - )) - if e.Application.Status.OperationState.SyncResult != nil { - for _, r := range e.Application.Status.OperationState.SyncResult.Resources { - namespacedName := r.Namespace + "/" + r.Name - switch r.Status { - case synccommon.ResultCodeSyncFailed, synccommon.ResultCodePruneSkipped: - b.WriteString(fmt.Sprintf("- %s `%s`: %s\n", r.Status, namespacedName, r.Message)) - } - } - } - return b.String() } if e.HealthIsChanged { @@ -129,3 +127,18 @@ func generateComment(e Event) string { return "" } + +func generateSyncResultComment(e Event) string { + if e.Application.Status.OperationState.SyncResult == nil { + return "" + } + var b strings.Builder + for _, r := range e.Application.Status.OperationState.SyncResult.Resources { + namespacedName := r.Namespace + "/" + r.Name + switch r.Status { + case synccommon.ResultCodeSyncFailed, synccommon.ResultCodePruneSkipped: + b.WriteString(fmt.Sprintf("- %s `%s`: %s\n", r.Status, namespacedName, r.Message)) + } + } + return b.String() +} From 6afe334bfadf892dca3ccdae3e1db59a6ca601e7 Mon Sep 17 00:00:00 2001 From: Hidetake Iwata Date: Thu, 22 Sep 2022 11:43:11 +0900 Subject: [PATCH 5/7] Make inactive on missing status --- .../applicationhealthdeployment_controller.go | 16 ++++++++++------ pkg/notification/deployment.go | 3 +++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/controllers/applicationhealthdeployment_controller.go b/controllers/applicationhealthdeployment_controller.go index 2d02a8a8..f4bc67ac 100644 --- a/controllers/applicationhealthdeployment_controller.go +++ b/controllers/applicationhealthdeployment_controller.go @@ -96,16 +96,20 @@ func (applicationHealthDeploymentComparer) Compare(applicationOld, applicationNe return false } - currentStatus := applicationNew.Status.Health.Status - if currentStatus != health.HealthStatusHealthy && currentStatus != health.HealthStatusDegraded { - return false - } - currentDeploymentURL := notification.GetDeploymentURL(applicationNew) if currentDeploymentURL == "" { return false } lastNotifiedDeploymentURL := applicationNew.Annotations[annotationNameOfLastDeploymentOfHealthy] - return currentDeploymentURL != lastNotifiedDeploymentURL + + switch applicationNew.Status.Health.Status { + case health.HealthStatusHealthy: + return currentDeploymentURL != lastNotifiedDeploymentURL + case health.HealthStatusDegraded: + return currentDeploymentURL != lastNotifiedDeploymentURL + case health.HealthStatusMissing: + return true + } + return false } diff --git a/pkg/notification/deployment.go b/pkg/notification/deployment.go index feab4a7c..6be21740 100644 --- a/pkg/notification/deployment.go +++ b/pkg/notification/deployment.go @@ -84,6 +84,9 @@ func generateDeploymentStatus(e Event) *github.DeploymentStatus { case health.HealthStatusDegraded: ds.State = "failure" return &ds + case health.HealthStatusMissing: + ds.State = "inactive" + return &ds } } From 8e624fdc6370161001b94486499abf92c135835e Mon Sep 17 00:00:00 2001 From: Hidetake Iwata Date: Thu, 22 Sep 2022 13:08:39 +0900 Subject: [PATCH 6/7] Don't handle terminating phase --- controllers/applicationphase_controller.go | 8 +++++++- pkg/notification/deployment.go | 3 --- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/controllers/applicationphase_controller.go b/controllers/applicationphase_controller.go index 9bae04a5..69e93efc 100644 --- a/controllers/applicationphase_controller.go +++ b/controllers/applicationphase_controller.go @@ -20,6 +20,7 @@ import ( "context" argocdv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + synccommon "github.com/argoproj/gitops-engine/pkg/sync/common" "github.com/int128/argocd-commenter/controllers/predicates" "github.com/int128/argocd-commenter/pkg/notification" "k8s.io/apimachinery/pkg/runtime" @@ -91,5 +92,10 @@ func (applicationPhaseComparer) Compare(applicationOld, applicationNew argocdv1a applicationOld.Status.OperationState.Phase == applicationNew.Status.OperationState.Phase { return false } - return true + + switch applicationNew.Status.OperationState.Phase { + case synccommon.OperationRunning, synccommon.OperationSucceeded, synccommon.OperationFailed, synccommon.OperationError: + return true + } + return false } diff --git a/pkg/notification/deployment.go b/pkg/notification/deployment.go index 6be21740..8d0fa4a0 100644 --- a/pkg/notification/deployment.go +++ b/pkg/notification/deployment.go @@ -66,9 +66,6 @@ func generateDeploymentStatus(e Event) *github.DeploymentStatus { case synccommon.OperationError: ds.State = "failure" return &ds - case synccommon.OperationTerminating: - ds.State = "inactive" - return &ds } } From 1ca5ba3b5985d199f1faa90c33c457c861c67435 Mon Sep 17 00:00:00 2001 From: Hidetake Iwata Date: Thu, 22 Sep 2022 13:11:21 +0900 Subject: [PATCH 7/7] Refactor --- controllers/applicationhealthcomment_controller.go | 13 ++++++------- .../applicationhealthdeployment_controller.go | 7 ++----- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/controllers/applicationhealthcomment_controller.go b/controllers/applicationhealthcomment_controller.go index 1e2342ac..51eaccbb 100644 --- a/controllers/applicationhealthcomment_controller.go +++ b/controllers/applicationhealthcomment_controller.go @@ -96,16 +96,15 @@ func (applicationHealthCommentComparer) Compare(applicationOld, applicationNew a return false } - currentStatus := applicationNew.Status.Health.Status - if currentStatus != health.HealthStatusHealthy && currentStatus != health.HealthStatusDegraded { - return false - } - currentDeployedRevision := getCurrentDeployedRevision(applicationNew) if currentDeployedRevision == "" { return false } - lastNotifiedRevision := applicationNew.Annotations[annotationNameOfLastRevisionOfHealthy] - return currentDeployedRevision != lastNotifiedRevision + switch applicationNew.Status.Health.Status { + case health.HealthStatusHealthy, health.HealthStatusDegraded: + lastNotifiedRevision := applicationNew.Annotations[annotationNameOfLastRevisionOfHealthy] + return currentDeployedRevision != lastNotifiedRevision + } + return false } diff --git a/controllers/applicationhealthdeployment_controller.go b/controllers/applicationhealthdeployment_controller.go index f4bc67ac..07049e3c 100644 --- a/controllers/applicationhealthdeployment_controller.go +++ b/controllers/applicationhealthdeployment_controller.go @@ -101,12 +101,9 @@ func (applicationHealthDeploymentComparer) Compare(applicationOld, applicationNe return false } - lastNotifiedDeploymentURL := applicationNew.Annotations[annotationNameOfLastDeploymentOfHealthy] - switch applicationNew.Status.Health.Status { - case health.HealthStatusHealthy: - return currentDeploymentURL != lastNotifiedDeploymentURL - case health.HealthStatusDegraded: + case health.HealthStatusHealthy, health.HealthStatusDegraded: + lastNotifiedDeploymentURL := applicationNew.Annotations[annotationNameOfLastDeploymentOfHealthy] return currentDeploymentURL != lastNotifiedDeploymentURL case health.HealthStatusMissing: return true