From 932d46e674aa66f52f3b915551e7edf55e46f51d Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Fri, 30 Aug 2024 16:35:07 +0200 Subject: [PATCH 01/12] Split too big comment per cluster If a "regular" aggrigated diff can't fit in a GH comment create one comment per cluster. There's still a fallback to concise diff (just lists changed objects) for extreme cases --- internal/pkg/githubapi/github.go | 86 ++++++++++++++++++------- templates/argoCD-diff-pr-comment.gotmpl | 3 + 2 files changed, 65 insertions(+), 24 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 76832c57..3cb5f4c6 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -30,6 +30,13 @@ import ( const githubCommentMaxSize = 65536 +type diffCommentData struct { + DiffOfChangedComponents []argocd.DiffResult + HasSyncableComponens bool + BranchName string + Header string +} + type promotionInstanceMetaData struct { SourcePath string `json:"sourcePath"` TargetPaths []string `json:"targetPaths"` @@ -169,11 +176,7 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } if len(diffOfChangedComponents) > 0 { - diffCommentData := struct { - DiffOfChangedComponents []argocd.DiffResult - HasSyncableComponens bool - BranchName string - }{ + diffCommentData := diffCommentData{ DiffOfChangedComponents: diffOfChangedComponents, BranchName: ghPrClientDetails.Ref, } @@ -184,25 +187,7 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr break } } - - err, templateOutput := executeTemplate(ghPrClientDetails.PrLogger, "argoCdDiff", "argoCD-diff-pr-comment.gotmpl", diffCommentData) - if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) - } else if len(templateOutput) > githubCommentMaxSize { - ghPrClientDetails.PrLogger.Warnf("Diff comment is too large (%d bytes), using concise template", len(templateOutput)) - err, templateOutput = executeTemplate(ghPrClientDetails.PrLogger, "argoCdDiffConcise", "argoCD-diff-pr-comment-concise.gotmpl", diffCommentData) - if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) - } - } - - err = commentPR(ghPrClientDetails, templateOutput) - if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Failed to comment ArgoCD diff: err=%s\n", err) - } + commentArgoCDdiff(ghPrClientDetails, diffCommentData) } else { ghPrClientDetails.PrLogger.Debugf("Diff not find affected ArogCD apps") } @@ -230,6 +215,59 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } } +func commentArgoCDdiff(ghPrClientDetails GhPrClientDetails, diffCommentData diffCommentData) (err error) { + + err, templateOutput := executeTemplate(ghPrClientDetails.PrLogger, "argoCdDiff", "argoCD-diff-pr-comment.gotmpl", diffCommentData) + if err != nil { + ghPrClientDetails.PrLogger.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) + return err + } + + // Happy path, the diff comment is small enough to be posted in one comment + if len(templateOutput) < githubCommentMaxSize { + err = commentPR(ghPrClientDetails, templateOutput) + return err + } + + // If the diff comment is too large, we'll split it into multiple comments, one per component + ghPrClientDetails.PrLogger.Warnf("Diff comment is too large (%d bytes), spiting to one comment per cluster", len(templateOutput)) + totalComponents := len(diffCommentData.DiffOfChangedComponents) + for i, singleComponentDiff := range diffCommentData.DiffOfChangedComponents { + // We take the diffCommentData and replace the DiffOfChangedComponents with a single component diff + componentTemplateData := diffCommentData + componentTemplateData.DiffOfChangedComponents = []argocd.DiffResult{singleComponentDiff} + // We also update the header to reflect the current component. + componentTemplateData.Header = fmt.Sprintf("Component %d/%d: %s(Splited for comment size)", i+1, totalComponents, singleComponentDiff.ComponentPath) + err, templateOutput := executeTemplate(ghPrClientDetails.PrLogger, "argoCdDiff", "argoCD-diff-pr-comment.gotmpl", componentTemplateData) + if err != nil { + ghPrClientDetails.PrLogger.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) + return err + } + // Even per component comments can be too large, in that case we'll just use the concise template + // Somewhat Happy path, the per-component diff comment is small enough to be posted in one comment + if len(templateOutput) < githubCommentMaxSize { + err = commentPR(ghPrClientDetails, templateOutput) + if err != nil { + return err + } + } + + // now we don't have much choice, this is the saddest path, we'll use the concise template + err, templateOutput = executeTemplate(ghPrClientDetails.PrLogger, "argoCdDiffConcise", "argoCD-diff-pr-comment-concise.gotmpl", componentTemplateData) + if err != nil { + ghPrClientDetails.PrLogger.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) + return err + } + err = commentPR(ghPrClientDetails, templateOutput) + if err != nil { + return err + } + + } + + return nil +} + // ReciveEventFile this one is similar to ReciveWebhook but it's used for CLI triggering, i simulates a webhook event to use the same code path as the webhook handler. func ReciveEventFile(eventType string, eventFilePath string, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair]) { log.Infof("Event type: %s", eventType) diff --git a/templates/argoCD-diff-pr-comment.gotmpl b/templates/argoCD-diff-pr-comment.gotmpl index d6745b83..043bffca 100644 --- a/templates/argoCD-diff-pr-comment.gotmpl +++ b/templates/argoCD-diff-pr-comment.gotmpl @@ -1,4 +1,7 @@ {{define "argoCdDiff"}} +{{ if .Header }} +{{ .Header }} +{{- end}} Diff of ArgoCD applications: {{ range $appDiffResult := .DiffOfChangedComponents }} From 0b6347b994046777dd7b2f2e3608be66c577059a Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Fri, 30 Aug 2024 16:42:11 +0200 Subject: [PATCH 02/12] Don't continue to sad path unless needed --- internal/pkg/githubapi/github.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 3cb5f4c6..8c23a97d 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -250,6 +250,7 @@ func commentArgoCDdiff(ghPrClientDetails GhPrClientDetails, diffCommentData diff if err != nil { return err } + continue } // now we don't have much choice, this is the saddest path, we'll use the concise template From 68d9fd0f4b632fc26acb7d204d3cbd63c5cf09fb Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Fri, 30 Aug 2024 16:54:57 +0200 Subject: [PATCH 03/12] Check return value --- internal/pkg/githubapi/github.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 8c23a97d..f9dd8f1f 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -187,7 +187,11 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr break } } - commentArgoCDdiff(ghPrClientDetails, diffCommentData) + err = commentArgoCDdiff(ghPrClientDetails, diffCommentData) + if err != nil { + prHandleError = err + ghPrClientDetails.PrLogger.Errorf("Failed to comment ArgoCD diff: err=%s\n", err) + } } else { ghPrClientDetails.PrLogger.Debugf("Diff not find affected ArogCD apps") } @@ -216,7 +220,6 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } func commentArgoCDdiff(ghPrClientDetails GhPrClientDetails, diffCommentData diffCommentData) (err error) { - err, templateOutput := executeTemplate(ghPrClientDetails.PrLogger, "argoCdDiff", "argoCD-diff-pr-comment.gotmpl", diffCommentData) if err != nil { ghPrClientDetails.PrLogger.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) @@ -263,7 +266,6 @@ func commentArgoCDdiff(ghPrClientDetails GhPrClientDetails, diffCommentData diff if err != nil { return err } - } return nil From 8ea0c3c41ac66de14b2e5b8dbd699fb751aa5f33 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Tue, 3 Sep 2024 16:47:31 +0200 Subject: [PATCH 04/12] Move logging out of executeTemplate to higher up the stack Create a new "testable" generateArgoCdDiffComments function to generate all the comments content Comment all the comments --- internal/pkg/githubapi/github.go | 61 +++++++++++++++------------ internal/pkg/githubapi/github_test.go | 37 ++++++++++++++++ internal/pkg/githubapi/promotion.go | 2 +- 3 files changed, 71 insertions(+), 29 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index f9dd8f1f..b34c4b25 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -30,7 +30,7 @@ import ( const githubCommentMaxSize = 65536 -type diffCommentData struct { +type DiffCommentData struct { DiffOfChangedComponents []argocd.DiffResult HasSyncableComponens bool BranchName string @@ -176,7 +176,7 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } if len(diffOfChangedComponents) > 0 { - diffCommentData := diffCommentData{ + diffCommentData := DiffCommentData{ DiffOfChangedComponents: diffOfChangedComponents, BranchName: ghPrClientDetails.Ref, } @@ -187,11 +187,18 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr break } } - err = commentArgoCDdiff(ghPrClientDetails, diffCommentData) + comments, err := generateArgoCdDiffComments(diffCommentData, githubCommentMaxSize) if err != nil { prHandleError = err ghPrClientDetails.PrLogger.Errorf("Failed to comment ArgoCD diff: err=%s\n", err) } + for _, comment := range comments { + err = commentPR(ghPrClientDetails, comment) + if err != nil { + prHandleError = err + ghPrClientDetails.PrLogger.Errorf("Failed to comment on PR: err=%s\n", err) + } + } } else { ghPrClientDetails.PrLogger.Debugf("Diff not find affected ArogCD apps") } @@ -219,21 +226,19 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } } -func commentArgoCDdiff(ghPrClientDetails GhPrClientDetails, diffCommentData diffCommentData) (err error) { - err, templateOutput := executeTemplate(ghPrClientDetails.PrLogger, "argoCdDiff", "argoCD-diff-pr-comment.gotmpl", diffCommentData) +func generateArgoCdDiffComments(diffCommentData DiffCommentData, githubCommentMaxSize int) (comments []string, err error) { + err, templateOutput := executeTemplate("argoCdDiff", "argoCD-diff-pr-comment.gotmpl", diffCommentData) if err != nil { - ghPrClientDetails.PrLogger.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) - return err + return comments, fmt.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) } // Happy path, the diff comment is small enough to be posted in one comment if len(templateOutput) < githubCommentMaxSize { - err = commentPR(ghPrClientDetails, templateOutput) - return err + comments = append(comments, templateOutput) + return comments, err } // If the diff comment is too large, we'll split it into multiple comments, one per component - ghPrClientDetails.PrLogger.Warnf("Diff comment is too large (%d bytes), spiting to one comment per cluster", len(templateOutput)) totalComponents := len(diffCommentData.DiffOfChangedComponents) for i, singleComponentDiff := range diffCommentData.DiffOfChangedComponents { // We take the diffCommentData and replace the DiffOfChangedComponents with a single component diff @@ -241,34 +246,32 @@ func commentArgoCDdiff(ghPrClientDetails GhPrClientDetails, diffCommentData diff componentTemplateData.DiffOfChangedComponents = []argocd.DiffResult{singleComponentDiff} // We also update the header to reflect the current component. componentTemplateData.Header = fmt.Sprintf("Component %d/%d: %s(Splited for comment size)", i+1, totalComponents, singleComponentDiff.ComponentPath) - err, templateOutput := executeTemplate(ghPrClientDetails.PrLogger, "argoCdDiff", "argoCD-diff-pr-comment.gotmpl", componentTemplateData) + err, templateOutput := executeTemplate("argoCdDiff", "argoCD-diff-pr-comment.gotmpl", componentTemplateData) if err != nil { - ghPrClientDetails.PrLogger.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) - return err + return comments, fmt.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) } // Even per component comments can be too large, in that case we'll just use the concise template // Somewhat Happy path, the per-component diff comment is small enough to be posted in one comment if len(templateOutput) < githubCommentMaxSize { - err = commentPR(ghPrClientDetails, templateOutput) + comments = append(comments, templateOutput) if err != nil { - return err + return comments, err } continue } // now we don't have much choice, this is the saddest path, we'll use the concise template - err, templateOutput = executeTemplate(ghPrClientDetails.PrLogger, "argoCdDiffConcise", "argoCD-diff-pr-comment-concise.gotmpl", componentTemplateData) + err, templateOutput = executeTemplate("argoCdDiffConcise", "argoCD-diff-pr-comment-concise.gotmpl", componentTemplateData) if err != nil { - ghPrClientDetails.PrLogger.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) - return err + return comments, fmt.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) } - err = commentPR(ghPrClientDetails, templateOutput) + comments = append(comments, templateOutput) if err != nil { - return err + return comments, err } } - return nil + return comments, nil } // ReciveEventFile this one is similar to ReciveWebhook but it's used for CLI triggering, i simulates a webhook event to use the same code path as the webhook handler. @@ -490,21 +493,23 @@ func handleCommentPrEvent(ghPrClientDetails GhPrClientDetails, ce *github.IssueC } func commentPlanInPR(ghPrClientDetails GhPrClientDetails, promotions map[string]PromotionInstance) { - _, templateOutput := executeTemplate(ghPrClientDetails.PrLogger, "dryRunMsg", "dry-run-pr-comment.gotmpl", promotions) + err, templateOutput := executeTemplate("dryRunMsg", "dry-run-pr-comment.gotmpl", promotions) + if err != nil { + ghPrClientDetails.PrLogger.Errorf("Failed to generate dry-run comment template: err=%s\n", err) + return + } _ = commentPR(ghPrClientDetails, templateOutput) } -func executeTemplate(logger *log.Entry, templateName string, templateFile string, data interface{}) (error, string) { +func executeTemplate(templateName string, templateFile string, data interface{}) (error, string) { var templateOutput bytes.Buffer messageTemplate, err := template.New(templateName).ParseFiles(getEnv("TEMPLATES_PATH", "templates/") + templateFile) if err != nil { - logger.Errorf("Failed to parse template: err=%v", err) - return err, "" + return fmt.Errorf("Failed to parse template: err=%v", err), "" } err = messageTemplate.ExecuteTemplate(&templateOutput, templateName, data) if err != nil { - logger.Errorf("Failed to execute template: err=%v", err) - return err, "" + return fmt.Errorf("Failed to execute template: err=%v", err), "" } return nil, templateOutput.String() } @@ -635,7 +640,7 @@ func handleMergedPrEvent(ghPrClientDetails GhPrClientDetails, prApproverGithubCl templateData := map[string]interface{}{ "prNumber": *pull.Number, } - err, templateOutput := executeTemplate(ghPrClientDetails.PrLogger, "autoMerge", "auto-merge-comment.gotmpl", templateData) + err, templateOutput := executeTemplate("autoMerge", "auto-merge-comment.gotmpl", templateData) if err != nil { return err } diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index d3634872..df131276 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -3,6 +3,8 @@ package githubapi import ( "bytes" "testing" + + "github.com/wayfair-incubator/telefonistka/internal/pkg/argocd" ) func TestGenerateSafePromotionBranchName(t *testing.T) { @@ -157,3 +159,38 @@ func TestIsSyncFromBranchAllowedForThisPath(t *testing.T) { }) } } + +func TestGenerateArgoCdDiffComments(t *testing.T) { + t.Parallel() + tests := map[string]struct { + diffCommentData DiffCommentData + expectedComments []string + maxCommentLength int + }{ + "Multiple diff Single comment": { + diffCommentData: DiffCommentData{ + DiffOfChangedComponents: []argocd.DiffResult{}, + HasSyncableComponens: true, + BranchName: "fooBar", + Header: "some Text", + }, + expectedComments: []string{ + "foo", + }, + }, + "Multiple diff Multiple comments": {}, + } + + for name, tc := range tests { + tc := tc // capture range variable + name := name + t.Run(name, func(t *testing.T) { + t.Parallel() + + result, _ := generateArgoCdDiffComments(tc.diffCommentData, tc.maxCommentLength) + if len(result) != len(tc.expectedComments) { + t.Errorf("%s: Expected result to be %v, got %v", name, tc.expectedComments, result) + } + }) + } +} diff --git a/internal/pkg/githubapi/promotion.go b/internal/pkg/githubapi/promotion.go index 561a9be0..298d73b0 100644 --- a/internal/pkg/githubapi/promotion.go +++ b/internal/pkg/githubapi/promotion.go @@ -76,7 +76,7 @@ func DetectDrift(ghPrClientDetails GhPrClientDetails) error { } } if len(diffOutputMap) != 0 { - err, templateOutput := executeTemplate(ghPrClientDetails.PrLogger, "driftMsg", "drift-pr-comment.gotmpl", diffOutputMap) + err, templateOutput := executeTemplate("driftMsg", "drift-pr-comment.gotmpl", diffOutputMap) if err != nil { return err } From 39fed16e4df4ed56c11b6b2abd5abcfd23559ae3 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 5 Sep 2024 13:40:08 +0200 Subject: [PATCH 05/12] Add test for 3 "levels" of comments --- Makefile | 2 +- internal/pkg/githubapi/github_test.go | 69 +++++++++++----- .../testdata/diff_comment_data_test.json | 80 +++++++++++++++++++ 3 files changed, 131 insertions(+), 20 deletions(-) create mode 100644 internal/pkg/githubapi/testdata/diff_comment_data_test.json diff --git a/Makefile b/Makefile index 26adb331..7089aff2 100644 --- a/Makefile +++ b/Makefile @@ -25,5 +25,5 @@ clean: .PHONY: test test: $(VENDOR_DIR) - go test -v -timeout 30s ./... + TEMPLATES_PATH=../../../templates/ go test -v -timeout 30s ./... diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index df131276..8d9cdf3b 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -2,9 +2,9 @@ package githubapi import ( "bytes" + "encoding/json" + "io/ioutil" "testing" - - "github.com/wayfair-incubator/telefonistka/internal/pkg/argocd" ) func TestGenerateSafePromotionBranchName(t *testing.T) { @@ -163,22 +163,25 @@ func TestIsSyncFromBranchAllowedForThisPath(t *testing.T) { func TestGenerateArgoCdDiffComments(t *testing.T) { t.Parallel() tests := map[string]struct { - diffCommentData DiffCommentData - expectedComments []string - maxCommentLength int + diffCommentDataTestDataFileName string + expectedNumberOfComments int + maxCommentLength int }{ - "Multiple diff Single comment": { - diffCommentData: DiffCommentData{ - DiffOfChangedComponents: []argocd.DiffResult{}, - HasSyncableComponens: true, - BranchName: "fooBar", - Header: "some Text", - }, - expectedComments: []string{ - "foo", - }, + "All cluster diffs fit in one comment": { + diffCommentDataTestDataFileName: "./testdata/diff_comment_data_test.json", + expectedNumberOfComments: 1, + maxCommentLength: 65535, + }, + "Split diffs, one cluster per comment": { + diffCommentDataTestDataFileName: "./testdata/diff_comment_data_test.json", + expectedNumberOfComments: 3, + maxCommentLength: 1000, + }, + "Split diffs, but maxCommentLength is very small so need to use the concise template": { + diffCommentDataTestDataFileName: "./testdata/diff_comment_data_test.json", + expectedNumberOfComments: 3, + maxCommentLength: 600, }, - "Multiple diff Multiple comments": {}, } for name, tc := range tests { @@ -186,11 +189,39 @@ func TestGenerateArgoCdDiffComments(t *testing.T) { name := name t.Run(name, func(t *testing.T) { t.Parallel() + var diffCommentData DiffCommentData + err := readJSONFromFile(tc.diffCommentDataTestDataFileName, &diffCommentData) + if err != nil { + t.Errorf("Error reading test data file: %s", err) + } - result, _ := generateArgoCdDiffComments(tc.diffCommentData, tc.maxCommentLength) - if len(result) != len(tc.expectedComments) { - t.Errorf("%s: Expected result to be %v, got %v", name, tc.expectedComments, result) + result, err := generateArgoCdDiffComments(diffCommentData, tc.maxCommentLength) + if err != nil { + t.Errorf("Error generating diff comments: %s", err) + } + if len(result) != tc.expectedNumberOfComments { + t.Errorf("%s: Expected number of comments to be %v, got %v", name, tc.expectedNumberOfComments, len(result)) + } + for _, comment := range result { + if len(comment) > tc.maxCommentLength { + t.Errorf("%s: Expected comment length to be less than %d, got %d", name, tc.maxCommentLength, len(comment)) + } } }) } } + +func readJSONFromFile(filename string, data interface{}) error { + // Read the JSON from the file + jsonData, err := ioutil.ReadFile(filename) + if err != nil { + return err + } + + // Unserialize the JSON into the provided struct + err = json.Unmarshal(jsonData, data) + if err != nil { + return err + } + return nil +} diff --git a/internal/pkg/githubapi/testdata/diff_comment_data_test.json b/internal/pkg/githubapi/testdata/diff_comment_data_test.json new file mode 100644 index 00000000..a67ece41 --- /dev/null +++ b/internal/pkg/githubapi/testdata/diff_comment_data_test.json @@ -0,0 +1,80 @@ + +{ + "DiffOfChangedComponents": [ + { + "ComponentPath": "clusters/playground/aws/eu-central-1/v1/special-delivery/ssllab-test/ssllab-test", + "ArgoCdAppName": "temp-ssllab-test-plg-aws-eu-central1-v1", + "ArgoCdAppURL": "https://argocd-lab.example.com/applications/temp-ssllab-test-plg-aws-eu-central1-v1", + "DiffElements": [ + { + "ObjectGroup": "", + "ObjectName": "ssllabs-exporter", + "ObjectKind": "Service", + "ObjectNamespace": "", + "Diff": " (*unstructured.Unstructured)(\n- \tnil,\n+ \t\u0026{\n+ \t\tObject: map[string]any{\n+ \t\t\t\"apiVersion\": string(\"v1\"),\n+ \t\t\t\"kind\": string(\"Service\"),\n+ \t\t\t\"metadata\": map[string]any{\"labels\": map[string]any{...}, \"name\": string(\"ssllabs-exporter\")},\n+ \t\t\t\"spec\": map[string]any{\n+ \t\t\t\t\"ports\": []any{...},\n+ \t\t\t\t\"selector\": map[string]any{...},\n+ \t\t\t\t\"type\": string(\"ClusterIP\"),\n+ \t\t\t},\n+ \t\t},\n+ \t},\n )\n" + }, + { + "ObjectGroup": "apps", + "ObjectName": "ssllabs-exporter", + "ObjectKind": "Deployment", + "ObjectNamespace": "", + "Diff": " (*unstructured.Unstructured)(\n- \tnil,\n+ \t\u0026{\n+ \t\tObject: map[string]any{\n+ \t\t\t\"apiVersion\": string(\"apps/v1\"),\n+ \t\t\t\"kind\": string(\"Deployment\"),\n+ \t\t\t\"metadata\": map[string]any{\"labels\": map[string]any{...}, \"name\": string(\"ssllabs-exporter\")},\n+ \t\t\t\"spec\": map[string]any{\n+ \t\t\t\t\"replicas\": int64(2),\n+ \t\t\t\t\"selector\": map[string]any{...},\n+ \t\t\t\t\"template\": map[string]any{...},\n+ \t\t\t},\n+ \t\t},\n+ \t},\n )\n" + } + ], + "HasDiff": true, + "DiffError": null, + "AppWasTemporarilyCreated": false + }, + { + "ComponentPath": "clusters/playground/aws/eu-central-1/v2/special-delivery/ssllab-test/ssllab-test", + "ArgoCdAppName": "temp-ssllab-test-plg-aws-eu-central1-v2", + "ArgoCdAppURL": "https://argocd-lab.example.com/applications/temp-ssllab-test-plg-aws-eu-central1-v1", + "DiffElements": [ + { + "ObjectGroup": "", + "ObjectName": "ssllabs-exporter", + "ObjectKind": "Service", + "ObjectNamespace": "", + "Diff": " (*unstructured.Unstructured)(\n- \tnil,\n+ \t\u0026{\n+ \t\tObject: map[string]any{\n+ \t\t\t\"apiVersion\": string(\"v1\"),\n+ \t\t\t\"kind\": string(\"Service\"),\n+ \t\t\t\"metadata\": map[string]any{\"labels\": map[string]any{...}, \"name\": string(\"ssllabs-exporter\")},\n+ \t\t\t\"spec\": map[string]any{\n+ \t\t\t\t\"ports\": []any{...},\n+ \t\t\t\t\"selector\": map[string]any{...},\n+ \t\t\t\t\"type\": string(\"ClusterIP\"),\n+ \t\t\t},\n+ \t\t},\n+ \t},\n )\n" + }, + { + "ObjectGroup": "apps", + "ObjectName": "ssllabs-exporter", + "ObjectKind": "Deployment", + "ObjectNamespace": "", + "Diff": " (*unstructured.Unstructured)(\n- \tnil,\n+ \t\u0026{\n+ \t\tObject: map[string]any{\n+ \t\t\t\"apiVersion\": string(\"apps/v1\"),\n+ \t\t\t\"kind\": string(\"Deployment\"),\n+ \t\t\t\"metadata\": map[string]any{\"labels\": map[string]any{...}, \"name\": string(\"ssllabs-exporter\")},\n+ \t\t\t\"spec\": map[string]any{\n+ \t\t\t\t\"replicas\": int64(2),\n+ \t\t\t\t\"selector\": map[string]any{...},\n+ \t\t\t\t\"template\": map[string]any{...},\n+ \t\t\t},\n+ \t\t},\n+ \t},\n )\n" + } + ], + "HasDiff": true, + "DiffError": null, + "AppWasTemporarilyCreated": false + }, + { + "ComponentPath": "clusters/playground/aws/eu-central-1/v3/special-delivery/ssllab-test/ssllab-test", + "ArgoCdAppName": "temp-ssllab-test-plg-aws-eu-central1-v3", + "ArgoCdAppURL": "https://argocd-lab.example.com/applications/temp-ssllab-test-plg-aws-eu-central1-v1", + "DiffElements": [ + { + "ObjectGroup": "", + "ObjectName": "ssllabs-exporter", + "ObjectKind": "Service", + "ObjectNamespace": "", + "Diff": " (*unstructured.Unstructured)(\n- \tnil,\n+ \t\u0026{\n+ \t\tObject: map[string]any{\n+ \t\t\t\"apiVersion\": string(\"v1\"),\n+ \t\t\t\"kind\": string(\"Service\"),\n+ \t\t\t\"metadata\": map[string]any{\"labels\": map[string]any{...}, \"name\": string(\"ssllabs-exporter\")},\n+ \t\t\t\"spec\": map[string]any{\n+ \t\t\t\t\"ports\": []any{...},\n+ \t\t\t\t\"selector\": map[string]any{...},\n+ \t\t\t\t\"type\": string(\"ClusterIP\"),\n+ \t\t\t},\n+ \t\t},\n+ \t},\n )\n" + }, + { + "ObjectGroup": "apps", + "ObjectName": "ssllabs-exporter", + "ObjectKind": "Deployment", + "ObjectNamespace": "", + "Diff": " (*unstructured.Unstructured)(\n- \tnil,\n+ \t\u0026{\n+ \t\tObject: map[string]any{\n+ \t\t\t\"apiVersion\": string(\"apps/v1\"),\n+ \t\t\t\"kind\": string(\"Deployment\"),\n+ \t\t\t\"metadata\": map[string]any{\"labels\": map[string]any{...}, \"name\": string(\"ssllabs-exporter\")},\n+ \t\t\t\"spec\": map[string]any{\n+ \t\t\t\t\"replicas\": int64(2),\n+ \t\t\t\t\"selector\": map[string]any{...},\n+ \t\t\t\t\"template\": map[string]any{...},\n+ \t\t\t},\n+ \t\t},\n+ \t},\n )\n" + } + ], + "HasDiff": true, + "DiffError": null, + "AppWasTemporarilyCreated": false + } + ], + "HasSyncableComponens": false, + "BranchName": "promotions/284-simulate-error-5c159151017f", + "Header": "" +} From ef24e05ad9f55b042fdce971b7b3d72bc328a3a4 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 5 Sep 2024 14:12:12 +0200 Subject: [PATCH 06/12] Replace depricated package --- internal/pkg/githubapi/github_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index 377a5f2a..0fa3bb0e 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -3,7 +3,6 @@ package githubapi import ( "bytes" "encoding/json" - "io/ioutil" "os" "testing" @@ -216,7 +215,7 @@ func TestGenerateArgoCdDiffComments(t *testing.T) { func readJSONFromFile(filename string, data interface{}) error { // Read the JSON from the file - jsonData, err := ioutil.ReadFile(filename) + jsonData, err := os.ReadFile(filename) if err != nil { return err } From fe297c4f182c62269e737ff7e483572f66e20709 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Tue, 10 Sep 2024 11:22:34 +0200 Subject: [PATCH 07/12] Apply suggestions from code review Co-authored-by: Hannes Gustafsson --- internal/pkg/githubapi/github.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index cfe1f033..48234f61 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -229,13 +229,13 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr func generateArgoCdDiffComments(diffCommentData DiffCommentData, githubCommentMaxSize int) (comments []string, err error) { err, templateOutput := executeTemplate("argoCdDiff", "argoCD-diff-pr-comment.gotmpl", diffCommentData) if err != nil { - return comments, fmt.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) + return nil, fmt.Errorf("failed to generate ArgoCD diff comment template: %w", err) } // Happy path, the diff comment is small enough to be posted in one comment if len(templateOutput) < githubCommentMaxSize { comments = append(comments, templateOutput) - return comments, err + return comments, nil } // If the diff comment is too large, we'll split it into multiple comments, one per component @@ -245,11 +245,12 @@ func generateArgoCdDiffComments(diffCommentData DiffCommentData, githubCommentMa componentTemplateData := diffCommentData componentTemplateData.DiffOfChangedComponents = []argocd.DiffResult{singleComponentDiff} // We also update the header to reflect the current component. - componentTemplateData.Header = fmt.Sprintf("Component %d/%d: %s(Splited for comment size)", i+1, totalComponents, singleComponentDiff.ComponentPath) + componentTemplateData.Header = fmt.Sprintf("Component %d/%d: %s (Split for comment size)", i+1, totalComponents, singleComponentDiff.ComponentPath) err, templateOutput := executeTemplate("argoCdDiff", "argoCD-diff-pr-comment.gotmpl", componentTemplateData) if err != nil { - return comments, fmt.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) + return comments, fmt.Errorf("failed to generate ArgoCD diff comment template: %w", err) } + // Even per component comments can be too large, in that case we'll just use the concise template // Somewhat Happy path, the per-component diff comment is small enough to be posted in one comment if len(templateOutput) < githubCommentMaxSize { @@ -263,12 +264,9 @@ func generateArgoCdDiffComments(diffCommentData DiffCommentData, githubCommentMa // now we don't have much choice, this is the saddest path, we'll use the concise template err, templateOutput = executeTemplate("argoCdDiffConcise", "argoCD-diff-pr-comment-concise.gotmpl", componentTemplateData) if err != nil { - return comments, fmt.Errorf("Failed to generate ArgoCD diff comment template: err=%s\n", err) + return comments, fmt.Errorf("failed to generate ArgoCD diff comment template: %w", err) } comments = append(comments, templateOutput) - if err != nil { - return comments, err - } } return comments, nil @@ -505,11 +503,11 @@ func executeTemplate(templateName string, templateFile string, data interface{}) var templateOutput bytes.Buffer messageTemplate, err := template.New(templateName).ParseFiles(getEnv("TEMPLATES_PATH", "templates/") + templateFile) if err != nil { - return fmt.Errorf("Failed to parse template: err=%v", err), "" + return fmt.Errorf("failed to parse template: %w", err), "" } err = messageTemplate.ExecuteTemplate(&templateOutput, templateName, data) if err != nil { - return fmt.Errorf("Failed to execute template: err=%v", err), "" + return fmt.Errorf("failed to execute template: %w", err), "" } return nil, templateOutput.String() } From 5003d5b7483986dca17c0a59515abe19264e9191 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Tue, 10 Sep 2024 11:23:45 +0200 Subject: [PATCH 08/12] Apply suggestions from code review Co-authored-by: Hannes Gustafsson --- internal/pkg/githubapi/github.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 48234f61..b3dd3bc0 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -244,6 +244,7 @@ func generateArgoCdDiffComments(diffCommentData DiffCommentData, githubCommentMa // We take the diffCommentData and replace the DiffOfChangedComponents with a single component diff componentTemplateData := diffCommentData componentTemplateData.DiffOfChangedComponents = []argocd.DiffResult{singleComponentDiff} + // We also update the header to reflect the current component. componentTemplateData.Header = fmt.Sprintf("Component %d/%d: %s (Split for comment size)", i+1, totalComponents, singleComponentDiff.ComponentPath) err, templateOutput := executeTemplate("argoCdDiff", "argoCD-diff-pr-comment.gotmpl", componentTemplateData) @@ -255,9 +256,6 @@ func generateArgoCdDiffComments(diffCommentData DiffCommentData, githubCommentMa // Somewhat Happy path, the per-component diff comment is small enough to be posted in one comment if len(templateOutput) < githubCommentMaxSize { comments = append(comments, templateOutput) - if err != nil { - return comments, err - } continue } From 3327c7b229de9dc3b845c6c00066596eacefb35d Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Tue, 10 Sep 2024 11:31:17 +0200 Subject: [PATCH 09/12] Fix typo in var name --- internal/pkg/githubapi/github.go | 4 ++-- internal/pkg/githubapi/testdata/diff_comment_data_test.json | 2 +- templates/argoCD-diff-pr-comment-concise.gotmpl | 2 +- templates/argoCD-diff-pr-comment.gotmpl | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index b3dd3bc0..09d8d70e 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -32,7 +32,7 @@ const githubCommentMaxSize = 65536 type DiffCommentData struct { DiffOfChangedComponents []argocd.DiffResult - HasSyncableComponens bool + HasSyncableComponents bool BranchName string Header string } @@ -183,7 +183,7 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr for _, componentPath := range componentPathList { if isSyncFromBranchAllowedForThisPath(config.Argocd.AllowSyncfromBranchPathRegex, componentPath) { - diffCommentData.HasSyncableComponens = true + diffCommentData.HasSyncableComponents = true break } } diff --git a/internal/pkg/githubapi/testdata/diff_comment_data_test.json b/internal/pkg/githubapi/testdata/diff_comment_data_test.json index a67ece41..6e181fe5 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 } ], - "HasSyncableComponens": false, + "HasSyncableComponents": 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 4a9fc14a..af379a74 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 .HasSyncableComponens }} +{{- if .HasSyncableComponents }} - [ ] Set ArgoCD apps Target Revision to `{{ .BranchName }}` diff --git a/templates/argoCD-diff-pr-comment.gotmpl b/templates/argoCD-diff-pr-comment.gotmpl index 043bffca..6d7ff360 100644 --- a/templates/argoCD-diff-pr-comment.gotmpl +++ b/templates/argoCD-diff-pr-comment.gotmpl @@ -43,7 +43,7 @@ It will not be present in ArgoCD UI for more than a few seconds and it can not b {{- end }} -{{- if .HasSyncableComponens }} +{{- if .HasSyncableComponents }} - [ ] Set ArgoCD apps Target Revision to `{{ .BranchName }}` From e482495d34e56bea78f8af4d1e76c2fef3718d2c Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Tue, 10 Sep 2024 11:44:00 +0200 Subject: [PATCH 10/12] Turn the readJSONFromFile to a test Helper --- internal/pkg/githubapi/github_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index 0fa3bb0e..e7371e7f 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -192,7 +192,7 @@ func TestGenerateArgoCdDiffComments(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() var diffCommentData DiffCommentData - err := readJSONFromFile(tc.diffCommentDataTestDataFileName, &diffCommentData) + err := readJSONFromFile(t, tc.diffCommentDataTestDataFileName, &diffCommentData) if err != nil { t.Errorf("Error reading test data file: %s", err) } @@ -213,7 +213,8 @@ func TestGenerateArgoCdDiffComments(t *testing.T) { } } -func readJSONFromFile(filename string, data interface{}) error { +func readJSONFromFile(t *testing.T, filename string, data interface{}) error { + t.Helper() // Read the JSON from the file jsonData, err := os.ReadFile(filename) if err != nil { From 3c6c902c11085fec8d95b8f18b748584ac466c10 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Tue, 10 Sep 2024 11:48:46 +0200 Subject: [PATCH 11/12] Lint issues --- internal/pkg/githubapi/github.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 09d8d70e..f8d1b70e 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -32,7 +32,7 @@ const githubCommentMaxSize = 65536 type DiffCommentData struct { DiffOfChangedComponents []argocd.DiffResult - HasSyncableComponents bool + HasSyncableComponents bool BranchName string Header string } @@ -244,14 +244,14 @@ func generateArgoCdDiffComments(diffCommentData DiffCommentData, githubCommentMa // We take the diffCommentData and replace the DiffOfChangedComponents with a single component diff componentTemplateData := diffCommentData componentTemplateData.DiffOfChangedComponents = []argocd.DiffResult{singleComponentDiff} - + // We also update the header to reflect the current component. componentTemplateData.Header = fmt.Sprintf("Component %d/%d: %s (Split for comment size)", i+1, totalComponents, singleComponentDiff.ComponentPath) err, templateOutput := executeTemplate("argoCdDiff", "argoCD-diff-pr-comment.gotmpl", componentTemplateData) if err != nil { return comments, fmt.Errorf("failed to generate ArgoCD diff comment template: %w", err) } - + // Even per component comments can be too large, in that case we'll just use the concise template // Somewhat Happy path, the per-component diff comment is small enough to be posted in one comment if len(templateOutput) < githubCommentMaxSize { From f79e70a4097ebadde1bb552bd39fad4fe3b3b235 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 12 Sep 2024 09:25:08 +0200 Subject: [PATCH 12/12] Apply suggestions from code review Co-authored-by: Hannes Gustafsson Remove some uneeded comments Simplify tests function Address lint issue --- internal/pkg/githubapi/github.go | 5 +---- internal/pkg/githubapi/github_test.go | 12 ++++-------- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index f8d1b70e..adc1b9b0 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -241,15 +241,12 @@ func generateArgoCdDiffComments(diffCommentData DiffCommentData, githubCommentMa // If the diff comment is too large, we'll split it into multiple comments, one per component totalComponents := len(diffCommentData.DiffOfChangedComponents) for i, singleComponentDiff := range diffCommentData.DiffOfChangedComponents { - // We take the diffCommentData and replace the DiffOfChangedComponents with a single component diff componentTemplateData := diffCommentData componentTemplateData.DiffOfChangedComponents = []argocd.DiffResult{singleComponentDiff} - - // We also update the header to reflect the current component. componentTemplateData.Header = fmt.Sprintf("Component %d/%d: %s (Split for comment size)", i+1, totalComponents, singleComponentDiff.ComponentPath) err, templateOutput := executeTemplate("argoCdDiff", "argoCD-diff-pr-comment.gotmpl", componentTemplateData) if err != nil { - return comments, fmt.Errorf("failed to generate ArgoCD diff comment template: %w", err) + return nil, fmt.Errorf("failed to generate ArgoCD diff comment template: %w", err) } // Even per component comments can be too large, in that case we'll just use the concise template diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index e7371e7f..a2d965aa 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -192,10 +192,7 @@ func TestGenerateArgoCdDiffComments(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() var diffCommentData DiffCommentData - err := readJSONFromFile(t, tc.diffCommentDataTestDataFileName, &diffCommentData) - if err != nil { - t.Errorf("Error reading test data file: %s", err) - } + readJSONFromFile(t, tc.diffCommentDataTestDataFileName, &diffCommentData) result, err := generateArgoCdDiffComments(diffCommentData, tc.maxCommentLength) if err != nil { @@ -213,20 +210,19 @@ func TestGenerateArgoCdDiffComments(t *testing.T) { } } -func readJSONFromFile(t *testing.T, filename string, data interface{}) error { +func readJSONFromFile(t *testing.T, filename string, data interface{}) { t.Helper() // Read the JSON from the file jsonData, err := os.ReadFile(filename) if err != nil { - return err + t.Fatalf("Error loading test data file: %s", err) } // Unserialize the JSON into the provided struct err = json.Unmarshal(jsonData, data) if err != nil { - return err + t.Fatalf("Error unmarshalling JSON: %s", err) } - return nil } func TestPrBody(t *testing.T) {