Skip to content

Commit

Permalink
Sd 805 improve diff comment (#32)
Browse files Browse the repository at this point in the history 
* Avoid displaying the "Sync from PR Branch" checkbox for new applications.
* 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 match its new usage
* Switch from Emoji based annotation to GH markdown "alerts"
  • Loading branch information
@Oded-B
Oded-B authored Oct 24, 2024
1 parent 33715e7 commit 81093e4
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 24 deletions.
37 changes: 27 additions & 10 deletions internal/pkg/githubapi/github.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,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 {
Expand Down Expand Up @@ -105,6 +105,27 @@ func (ghPrClientDetails *GhPrClientDetails) getBlameURLPrefix() string {
return fmt.Sprintf("%s/%s/%s/blame", githubHost, ghPrClientDetails.Owner, ghPrClientDetails.Repo)
}

// shouldSyncBranchCheckBoxBeDisplayed checks if the sync branch checkbox should be displayed in the PR comment.
// The checkbox should be displayed if:
// - The component is allowed to be synced from a branch(based on Telefonsitka configuration)
// - The relevant app is not new, temporary app that was created just to generate the diff
func shouldSyncBranchCheckBoxBeDisplayed(componentPathList []string, allowSyncfromBranchPathRegex string, diffOfChangedComponents []argocd.DiffResult) bool {
for _, componentPath := range componentPathList {
// First we check if the component is allowed to be synced from a branch
if !isSyncFromBranchAllowedForThisPath(allowSyncfromBranchPathRegex, componentPath) {
continue
}
// Then we check the relevant app is not new, temporary app.
// We don't support syncing new apps from branches
for _, diffOfChangedComponent := range diffOfChangedComponents {
if diffOfChangedComponent.ComponentPath == componentPath && !diffOfChangedComponent.AppWasTemporarilyCreated {
return true
}
}
}
return false
}

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
Expand Down Expand Up @@ -193,12 +214,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
Expand Down
54 changes: 54 additions & 0 deletions internal/pkg/githubapi/github_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"time"

"github.com/stretchr/testify/assert"
"github.com/wayfair-incubator/telefonistka/internal/pkg/argocd"
)

func TestGenerateSafePromotionBranchName(t *testing.T) {
Expand Down Expand Up @@ -294,6 +295,59 @@ func TestGhPrClientDetailsGetBlameURLPrefix(t *testing.T) {
}
}

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)
}
}

func TestCommitStatusTargetURL(t *testing.T) {
t.Parallel()

Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/githubapi/testdata/diff_comment_data_test.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@
"AppWasTemporarilyCreated": false
}
],
"HasSyncableComponents": false,
"DisplaySyncBranchCheckBox": false,
"BranchName": "promotions/284-simulate-error-5c159151017f",
"Header": ""
}
13 changes: 7 additions & 6 deletions templates/argoCD-diff-pr-comment-concise.gotmpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ Diff of ArgoCD applications(⚠️ concise view, full diff didn't fit GH comment


{{if $appDiffResult.DiffError }}
⚠️ ⚠️ **Error getting diff from ArgoCD** (`{{ $appDiffResult.ComponentPath }}`) ⚠️ ⚠️
> [!CAUTION]
> **Error getting diff from ArgoCD** (`{{ $appDiffResult.ComponentPath }}`)

```
{{ $appDiffResult.DiffError }}

Expand All @@ -27,17 +29,16 @@ Diff of ArgoCD applications(⚠️ concise view, full diff didn't fit GH comment
No diff 🤷
{{- end}}
{{if $appDiffResult.AppWasTemporarilyCreated }}
⚠️ ⚠️ ⚠️
This PR appears to create this new application, Telefonistka has **temporarly** created an ArgoCD app object for it just to render its manifests.
It will not be present in ArgoCD UI for more than a few seconds and it can not be synced from the PR branch
⚠️ ⚠️ ⚠️
> [!NOTE]
> This PR appears to create this new application, Telefonistka has **temporarly** created an ArgoCD app object for it just to render its manifests.
> It will not be present in ArgoCD UI for more than a few seconds.
{{- end}}

{{- end }}

{{- end }}

{{- if .HasSyncableComponents }}
{{- if .DisplaySyncBranchCheckBox }}

- [ ] <!-- telefonistka-argocd-branch-sync --> Set ArgoCD apps Target Revision to `{{ .BranchName }}`

Expand Down
17 changes: 10 additions & 7 deletions templates/argoCD-diff-pr-comment.gotmpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ Diff of ArgoCD applications:


{{if $appDiffResult.DiffError }}
⚠️ ⚠️ **Error getting diff from ArgoCD** (`{{ $appDiffResult.ComponentPath }}`) ⚠️ ⚠️
> [!CAUTION]
> **Error getting diff from ArgoCD** (`{{ $appDiffResult.ComponentPath }}`)

Please check the App Conditions of <img src="https://argo-cd.readthedocs.io/en/stable/assets/favicon.png" width="20"/> **[{{ $appDiffResult.ArgoCdAppName }}]({{ $appDiffResult.ArgoCdAppURL }})** for more details.
{{- if $appDiffResult.AppWasTemporarilyCreated }}
⚠️ For investigation we kept the temporary application, please make sure to clean it up later! ⚠️
> [!WARNING]
> For investigation we kept the temporary application, please make sure to clean it up later!

{{- end}}
```
{{ $appDiffResult.DiffError }}
Expand All @@ -37,17 +41,16 @@ Please check the App Conditions of <img src="https://argo-cd.readthedocs.io/en/s
No diff 🤷
{{- end}}
{{if $appDiffResult.AppWasTemporarilyCreated }}
⚠️ ⚠️ ⚠️
This PR appears to create this new application, Telefonistka has **temporarly** created an ArgoCD app object for it just to render its manifests.
It will not be present in ArgoCD UI for more than a few seconds and it can not be synced from the PR branch
⚠️ ⚠️ ⚠️
> [!NOTE]
> This PR appears to create this new application, Telefonistka has **temporarly** created an ArgoCD app object for it just to render its manifests.
> It will not be present in ArgoCD UI for more than a few seconds.
{{- end}}

{{- end }}

{{- end }}

{{- if .HasSyncableComponents }}
{{- if .DisplaySyncBranchCheckBox }}

- [ ] <!-- telefonistka-argocd-branch-sync --> Set ArgoCD apps Target Revision to `{{ .BranchName }}`

Expand Down

0 comments on commit 81093e4

Please sign in to comment.