From 003166a7f6c76fa1d69c665ac8a92b82d44a4682 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Wed, 23 Oct 2024 13:23:54 +0200 Subject: [PATCH] Avoid displaying the "Sync from PR Branch" checkbox for new applications. match its new usage * Create a new function to check if all application in a PR are new. * Write tests for new function * Rename HasSyncableComponents to DisplaySyncBranchCheckBox to better --- internal/pkg/githubapi/github.go | 40 ++++++++++---- internal/pkg/githubapi/github_test.go | 54 +++++++++++++++++++ .../testdata/diff_comment_data_test.json | 2 +- .../argoCD-diff-pr-comment-concise.gotmpl | 2 +- templates/argoCD-diff-pr-comment.gotmpl | 2 +- 5 files changed, 87 insertions(+), 13 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index f9be31a7..0046b8cb 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -34,10 +34,10 @@ const ( ) type DiffCommentData struct { - DiffOfChangedComponents []argocd.DiffResult - HasSyncableComponents bool - BranchName string - Header string + DiffOfChangedComponents []argocd.DiffResult + DisplaySyncBranchCheckBox bool + BranchName string + Header string } type promotionInstanceMetaData struct { @@ -104,6 +104,30 @@ func (ghPrClientDetails *GhPrClientDetails) getBlameURLPrefix() string { return fmt.Sprintf("%s/%s/%s/blame", githubHost, ghPrClientDetails.Owner, ghPrClientDetails.Repo) } +// This function is used to check if we should display the sync branch checkbox in the PR comment. +// This depends on the allowSyncfromBranchPathRegex configuration and whether the component is new or not. +func shouldSyncBranchCheckBoxBeDisplayed(componentPathList []string, allowSyncfromBranchPathRegex string, diffOfChangedComponents []argocd.DiffResult) bool { + var displaySyncBranchCheckBox bool + +out: + for _, componentPath := range componentPathList { + if isSyncFromBranchAllowedForThisPath(allowSyncfromBranchPathRegex, componentPath) { + // Here we are checking if the syncable component is not a new app that was temporarily created. + // We don't support syncing new apps from branches + for _, diffOfChangedComponent := range diffOfChangedComponents { + if diffOfChangedComponent.ComponentPath == componentPath { + if !diffOfChangedComponent.AppWasTemporarilyCreated { + displaySyncBranchCheckBox = true + break out + } + } + } + } + } + + return displaySyncBranchCheckBox +} + func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPrClientDetails, mainGithubClientPair GhClientPair, approverGithubClientPair GhClientPair, ctx context.Context) { ghPrClientDetails.getPrMetadata(eventPayload.PullRequest.GetBody()) // wasCommitStatusSet and the placement of SetCommitStatus in the flow is used to ensure an API call is only made where it needed @@ -192,12 +216,8 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr BranchName: ghPrClientDetails.Ref, } - for _, componentPath := range componentPathList { - if isSyncFromBranchAllowedForThisPath(config.Argocd.AllowSyncfromBranchPathRegex, componentPath) { - diffCommentData.HasSyncableComponents = true - break - } - } + diffCommentData.DisplaySyncBranchCheckBox = shouldSyncBranchCheckBoxBeDisplayed(componentPathList, config.Argocd.AllowSyncfromBranchPathRegex, diffOfChangedComponents) + comments, err := generateArgoCdDiffComments(diffCommentData, githubCommentMaxSize) if err != nil { prHandleError = err diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index 3814e2b8..1ac3e6e7 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/wayfair-incubator/telefonistka/internal/pkg/argocd" ) func TestGenerateSafePromotionBranchName(t *testing.T) { @@ -289,3 +290,56 @@ func TestGhPrClientDetailsGetBlameURLPrefix(t *testing.T) { assert.Equal(t, tc.ExpectURL, blameURLPrefix) } } + +func TestShouldSyncBranchCheckBoxBeDisplayed(t *testing.T) { + t.Parallel() + tests := map[string]struct { + componentPathList []string + allowSyncfromBranchPathRegex string + diffOfChangedComponents []argocd.DiffResult + expected bool + }{ + "New App": { + componentPathList: []string{"workspace/app1"}, + allowSyncfromBranchPathRegex: `^workspace/.*$`, + diffOfChangedComponents: []argocd.DiffResult{ + { + AppWasTemporarilyCreated: true, + ComponentPath: "workspace/app1", + }, + }, + expected: false, + }, + "Existing App": { + componentPathList: []string{"workspace/app1"}, + allowSyncfromBranchPathRegex: `^workspace/.*$`, + diffOfChangedComponents: []argocd.DiffResult{ + { + AppWasTemporarilyCreated: false, + ComponentPath: "workspace/app1", + }, + }, + expected: true, + }, + "Mixed New and Existing Apps": { + componentPathList: []string{"workspace/app1", "workspace/app2"}, + allowSyncfromBranchPathRegex: `^workspace/.*$`, + diffOfChangedComponents: []argocd.DiffResult{ + { + AppWasTemporarilyCreated: false, + ComponentPath: "workspace/app1", + }, + { + AppWasTemporarilyCreated: true, + ComponentPath: "workspace/app2", + }, + }, + expected: true, + }, + } + + for i, tc := range tests { + result := shouldSyncBranchCheckBoxBeDisplayed(tc.componentPathList, tc.allowSyncfromBranchPathRegex, tc.diffOfChangedComponents) + assert.Equal(t, tc.expected, result, i) + } +} diff --git a/internal/pkg/githubapi/testdata/diff_comment_data_test.json b/internal/pkg/githubapi/testdata/diff_comment_data_test.json index 6e181fe5..37ab5c56 100644 --- a/internal/pkg/githubapi/testdata/diff_comment_data_test.json +++ b/internal/pkg/githubapi/testdata/diff_comment_data_test.json @@ -74,7 +74,7 @@ "AppWasTemporarilyCreated": false } ], - "HasSyncableComponents": false, + "DisplaySyncBranchCheckBox": false, "BranchName": "promotions/284-simulate-error-5c159151017f", "Header": "" } diff --git a/templates/argoCD-diff-pr-comment-concise.gotmpl b/templates/argoCD-diff-pr-comment-concise.gotmpl index af379a74..c5a6cab9 100644 --- a/templates/argoCD-diff-pr-comment-concise.gotmpl +++ b/templates/argoCD-diff-pr-comment-concise.gotmpl @@ -37,7 +37,7 @@ It will not be present in ArgoCD UI for more than a few seconds and it can not b {{- end }} -{{- if .HasSyncableComponents }} +{{- if .DisplaySyncBranchCheckBox }} - [ ] Set ArgoCD apps Target Revision to `{{ .BranchName }}` diff --git a/templates/argoCD-diff-pr-comment.gotmpl b/templates/argoCD-diff-pr-comment.gotmpl index 04705596..e3392c5c 100644 --- a/templates/argoCD-diff-pr-comment.gotmpl +++ b/templates/argoCD-diff-pr-comment.gotmpl @@ -47,7 +47,7 @@ It will not be present in ArgoCD UI for more than a few seconds and it can not b {{- end }} -{{- if .HasSyncableComponents }} +{{- if .DisplaySyncBranchCheckBox }} - [ ] Set ArgoCD apps Target Revision to `{{ .BranchName }}`