Split too big comment per cluster (#22)

* Split too big comment per cluster If a "regular" aggregate 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 * 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 * Add test for 3 "levels" of comments * Apply suggestions from code review Co-authored-by: Hannes Gustafsson <hnnsgstfssn@gmail.com>
commercetools · Sep 12, 2024 · d60f34e · d60f34e
1 parent bbe49ea
commit d60f34e
Show file tree

Hide file tree

Showing 7 changed files with 216 additions and 30 deletions.
diff --git 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.go b/internal/pkg/githubapi/github.go
@@ -30,6 +30,13 @@ import (
 
 const githubCommentMaxSize = 65536
 
+type DiffCommentData struct {
+	DiffOfChangedComponents []argocd.DiffResult
+	HasSyncableComponents   bool
+	BranchName              string
+	Header                  string
+}
+
 type promotionInstanceMetaData struct {
 	SourcePath  string   `json:"sourcePath"`
 	TargetPaths []string `json:"targetPaths"`
@@ -169,40 +176,29 @@ 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,
 				}
 
 				for _, componentPath := range componentPathList {
 					if isSyncFromBranchAllowedForThisPath(config.Argocd.AllowSyncfromBranchPathRegex, componentPath) {
-						diffCommentData.HasSyncableComponens = true
+						diffCommentData.HasSyncableComponents = true
 						break
 					}
 				}
-
-				err, templateOutput := executeTemplate(ghPrClientDetails.PrLogger, "argoCdDiff", "argoCD-diff-pr-comment.gotmpl", diffCommentData)
+				comments, err := generateArgoCdDiffComments(diffCommentData, githubCommentMaxSize)
 				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)
+					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 generate ArgoCD diff comment template: err=%s\n", err)
+						ghPrClientDetails.PrLogger.Errorf("Failed to comment on PR: 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)
-				}
 			} else {
 				ghPrClientDetails.PrLogger.Debugf("Diff not find affected ArogCD apps")
 			}
@@ -230,6 +226,47 @@ 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 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, nil
+	}
+
+	// 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 {
+		componentTemplateData := diffCommentData
+		componentTemplateData.DiffOfChangedComponents = []argocd.DiffResult{singleComponentDiff}
+		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 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
+		// 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)
+			continue
+		}
+
+		// 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: %w", err)
+		}
+		comments = append(comments, templateOutput)
+	}
+
+	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.
 func ReciveEventFile(eventType string, eventFilePath string, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair]) {
 	log.Infof("Event type: %s", eventType)
@@ -449,21 +486,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: %w", 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: %w", err), ""
 	}
 	return nil, templateOutput.String()
 }
@@ -594,7 +633,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
@@ -2,6 +2,7 @@ package githubapi
 
 import (
 	"bytes"
+	"encoding/json"
 	"os"
 	"testing"
 
@@ -161,6 +162,69 @@ func TestIsSyncFromBranchAllowedForThisPath(t *testing.T) {
 	}
 }
 
+func TestGenerateArgoCdDiffComments(t *testing.T) {
+	t.Parallel()
+	tests := map[string]struct {
+		diffCommentDataTestDataFileName string
+		expectedNumberOfComments        int
+		maxCommentLength                int
+	}{
+		"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,
+		},
+	}
+
+	for name, tc := range tests {
+		tc := tc // capture range variable
+		name := name
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			var diffCommentData DiffCommentData
+			readJSONFromFile(t, tc.diffCommentDataTestDataFileName, &diffCommentData)
+
+			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(t *testing.T, filename string, data interface{}) {
+	t.Helper()
+	// Read the JSON from the file
+	jsonData, err := os.ReadFile(filename)
+	if err != nil {
+		t.Fatalf("Error loading test data file: %s", err)
+	}
+
+	// Unserialize the JSON into the provided struct
+	err = json.Unmarshal(jsonData, data)
+	if err != nil {
+		t.Fatalf("Error unmarshalling JSON: %s", err)
+	}
+}
+
 func TestPrBody(t *testing.T) {
 	t.Parallel()
 	keys := []int{1, 2, 3}

diff --git 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
 		}

diff --git a/internal/pkg/githubapi/testdata/diff_comment_data_test.json 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
+    }
+  ],
+  "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
@@ -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 }}
 
 - [ ] <!-- telefonistka-argocd-branch-sync --> Set ArgoCD apps Target Revision to `{{ .BranchName }}`  
 

diff --git 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 }}
 
@@ -44,7 +47,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 }}
 
 - [ ] <!-- telefonistka-argocd-branch-sync --> Set ArgoCD apps Target Revision to `{{ .BranchName }}`