From deb0692122a0a704bebb8b82ca68203023fe5992 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 30 Oct 2024 05:13:24 -0400 Subject: [PATCH] SD-770 fixup unique target paths and multi promotion (#37) Promotion PR description generated by Telefonistka contains all default promotion targets for a certain promotion path. This updates the prBody function to only list the promotion targets to which the promotion is supposed to happen for a certain promotion path. * Added a filterSkipPaths function when generating a pr comment. This will take the targetPaths and filter out any skipped paths from the final promotion pr comment. * Add getPromotionSkipPaths to find the component with fewest skip paths --------- Co-authored-by: Hannes Gustafsson --- internal/pkg/githubapi/github.go | 92 +++++++++++---- internal/pkg/githubapi/github_test.go | 105 +++++++++++------- .../pr_body_multi_component.golden.md | 4 + 3 files changed, 137 insertions(+), 64 deletions(-) create mode 100644 internal/pkg/githubapi/testdata/pr_body_multi_component.golden.md diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index b7a1fe81..8192e6ca 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -2,6 +2,7 @@ package githubapi import ( "bytes" + "cmp" "context" "crypto/sha1" //nolint:gosec // G505: Blocklisted import crypto/sha1: weak cryptographic primitive (gosec), this is not a cryptographic use case "encoding/base64" @@ -14,6 +15,7 @@ import ( "path" "path/filepath" "regexp" + "slices" "sort" "strings" "text/template" @@ -1126,6 +1128,8 @@ func generatePromotionPrBody(ghPrClientDetails GhPrClientDetails, components str newPrMetadata.PromotedPaths = maps.Keys(promotion.ComputedSyncPaths) + promotionSkipPaths := getPromotionSkipPaths(promotion) + newPrBody = fmt.Sprintf("Promotion path(%s):\n\n", components) keys := make([]int, 0) @@ -1133,7 +1137,8 @@ func generatePromotionPrBody(ghPrClientDetails GhPrClientDetails, components str keys = append(keys, k) } sort.Ints(keys) - newPrBody = prBody(keys, newPrMetadata, newPrBody) + + newPrBody = prBody(keys, newPrMetadata, newPrBody, promotionSkipPaths) prMetadataString, _ := newPrMetadata.serialize() @@ -1142,14 +1147,59 @@ func generatePromotionPrBody(ghPrClientDetails GhPrClientDetails, components str return newPrBody } -func prBody(keys []int, newPrMetadata prMetadata, newPrBody string) string { +// getPromotionSkipPaths returns a map of paths that are marked as skipped for this promotion +// when we have multiple components, we are going to use the component that has the fewest skip paths +func getPromotionSkipPaths(promotion PromotionInstance) map[string]bool { + perComponentSkippedTargetPaths := promotion.Metadata.PerComponentSkippedTargetPaths + promotionSkipPaths := map[string]bool{} + + if len(perComponentSkippedTargetPaths) == 0 { + return promotionSkipPaths + } + + // if any promoted component is not in the perComponentSkippedTargetPaths + // then that means we have a component that is promoted to all paths, + // therefore, we return an empty promotionSkipPaths map to signify that + // there are no paths that are skipped for this promotion + for _, component := range promotion.Metadata.ComponentNames { + if _, ok := perComponentSkippedTargetPaths[component]; !ok { + return promotionSkipPaths + } + } + + // if we have one or more components then we are just going to + // user the component that has the fewest skipPaths when + // generating the promotion prBody. This way the promotion + // body will error on the side of informing the user + // of more promotion paths, rather than leaving some out. + skipCounts := map[string]int{} + for component, paths := range perComponentSkippedTargetPaths { + skipCounts[component] = len(paths) + } + + skipPaths := maps.Keys(skipCounts) + slices.SortFunc(skipPaths, func(a, b string) int { + return cmp.Compare(skipCounts[a], skipCounts[b]) + }) + + componentWithFewestSkippedPaths := skipPaths[0] + for _, p := range perComponentSkippedTargetPaths[componentWithFewestSkippedPaths] { + promotionSkipPaths[p] = true + } + + return promotionSkipPaths +} + +func prBody(keys []int, newPrMetadata prMetadata, newPrBody string, promotionSkipPaths map[string]bool) string { const mkTab = "    " sp := "" tp := "" for i, k := range keys { sp = newPrMetadata.PreviousPromotionMetadata[k].SourcePath - x := identifyCommonPaths(newPrMetadata.PromotedPaths, newPrMetadata.PreviousPromotionMetadata[k].TargetPaths) + x := filterSkipPaths(newPrMetadata.PreviousPromotionMetadata[k].TargetPaths, promotionSkipPaths) + // sort the paths so that we have a predictable order for tests and better readability for users + sort.Strings(x) tp = strings.Join(x, fmt.Sprintf("` \n%s`", strings.Repeat(mkTab, i+1))) newPrBody = newPrBody + fmt.Sprintf("%s↘️ #%d `%s` ➡️ \n%s`%s` \n", strings.Repeat(mkTab, i), k, sp, strings.Repeat(mkTab, i+1), tp) } @@ -1157,30 +1207,26 @@ func prBody(keys []int, newPrMetadata prMetadata, newPrBody string) string { return newPrBody } -// identifyCommonPaths takes a slice of promotion paths and target paths and -// returns a slice containing paths in common. -func identifyCommonPaths(promotionPaths []string, targetPaths []string) []string { - if (len(promotionPaths) == 0) || (len(targetPaths) == 0) { - return nil - } - var commonPaths []string - for _, pp := range promotionPaths { - if pp == "" { - continue +// filterSkipPaths filters out the paths that are marked as skipped +func filterSkipPaths(targetPaths []string, promotionSkipPaths map[string]bool) []string { + pathSkip := make(map[string]bool) + for _, targetPath := range targetPaths { + if _, ok := promotionSkipPaths[targetPath]; ok { + pathSkip[targetPath] = true + } else { + pathSkip[targetPath] = false } - for _, tp := range targetPaths { - if tp == "" { - continue - } - // strings.HasPrefix is used to check that the target path and promotion path match instead of - // using 'pp == tp' because the promotion path is targetPath + component. - if strings.HasPrefix(pp, tp) { - commonPaths = append(commonPaths, tp) - } + } + + var paths []string + + for path, skip := range pathSkip { + if !skip { + paths = append(paths, path) } } - return commonPaths + return paths } func createPrObject(ghPrClientDetails GhPrClientDetails, newBranchRef string, newPrTitle string, newPrBody string, defaultBranch string, assignee string) (*github.PullRequest, error) { diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index c5d6caf8..48e24e9f 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -235,6 +235,7 @@ func readJSONFromFile(t *testing.T, filename string, data interface{}) { func TestPrBody(t *testing.T) { t.Parallel() keys := []int{1, 2, 3} + promotionSkipPaths := map[string]bool{"targetPath3": true} newPrMetadata := prMetadata{ // note: "targetPath3" is missing from the list of promoted paths, so it should not // be included in the new PR body. @@ -254,7 +255,7 @@ func TestPrBody(t *testing.T) { }, }, } - newPrBody := prBody(keys, newPrMetadata, "") + newPrBody := prBody(keys, newPrMetadata, "", promotionSkipPaths) expectedPrBody, err := os.ReadFile("testdata/pr_body.golden.md") if err != nil { t.Fatalf("Error loading golden file: %s", err) @@ -262,6 +263,33 @@ func TestPrBody(t *testing.T) { assert.Equal(t, string(expectedPrBody), newPrBody) } +func TestPrBodyMultiComponent(t *testing.T) { + t.Parallel() + keys := []int{1, 2} + promotionSkipPaths := map[string]bool{} + newPrMetadata := prMetadata{ + // note: "targetPath3" is missing from the list of promoted paths, so it should not + // be included in the new PR body. + PromotedPaths: []string{"targetPath1/component1", "targetPath1/component2", "targetPath2/component1"}, + PreviousPromotionMetadata: map[int]promotionInstanceMetaData{ + 1: { + SourcePath: "sourcePath1", + TargetPaths: []string{"targetPath1"}, + }, + 2: { + SourcePath: "sourcePath2", + TargetPaths: []string{"targetPath2"}, + }, + }, + } + newPrBody := prBody(keys, newPrMetadata, "", promotionSkipPaths) + expectedPrBody, err := os.ReadFile("testdata/pr_body_multi_component.golden.md") + if err != nil { + t.Fatalf("Error loading golden file: %s", err) + } + assert.Equal(t, string(expectedPrBody), newPrBody) +} + func TestGhPrClientDetailsGetBlameURLPrefix(t *testing.T) { t.Parallel() tests := []struct { @@ -408,71 +436,66 @@ func TestCommitStatusTargetURL(t *testing.T) { } } -func Test_identifyCommonPaths(t *testing.T) { +func Test_getPromotionSkipPaths(t *testing.T) { t.Parallel() type args struct { - promoPaths []string - targetPaths []string + promotion PromotionInstance } tests := []struct { name string args args - want []string + want map[string]bool }{ { - name: "same paths", - args: args{ - promoPaths: []string{"path1/component/path", "path2/component/path", "path3/component/path"}, - targetPaths: []string{"path1", "path2", "path3"}, - }, - want: []string{"path1", "path2", "path3"}, - }, - { - name: "paths1 is empty", + name: "No skip paths", args: args{ - promoPaths: []string{}, - targetPaths: []string{"path1", "path2", "path3"}, + promotion: PromotionInstance{ + Metadata: PromotionInstanceMetaData{ + PerComponentSkippedTargetPaths: map[string][]string{}, + }, + }, }, - want: nil, + want: map[string]bool{}, }, { - name: "paths2 is empty", + name: "one skip path", args: args{ - promoPaths: []string{"path1/component/some", "path2/some/other", "path3"}, - targetPaths: []string{}, + promotion: PromotionInstance{ + Metadata: PromotionInstanceMetaData{ + PerComponentSkippedTargetPaths: map[string][]string{ + "component1": {"targetPath1", "targetPath2"}, + }, + }, + }, }, - want: nil, - }, - { - name: "paths2 missing elements", - args: args{ - promoPaths: []string{"path1", "path2", "path3"}, - targetPaths: []string{""}, + want: map[string]bool{ + "targetPath1": true, + "targetPath2": true, }, - want: nil, }, { - name: "path1 missing elements", + name: "multiple skip path", args: args{ - promoPaths: []string{""}, - targetPaths: []string{"path1", "path2"}, + promotion: PromotionInstance{ + Metadata: PromotionInstanceMetaData{ + PerComponentSkippedTargetPaths: map[string][]string{ + "component1": {"targetPath1", "targetPath2", "targetPath3"}, + "component2": {"targetPath3"}, + "component3": {"targetPath1", "targetPath2"}, + }, + }, + }, }, - want: nil, - }, - { - name: "path1 and path2 common elements", - args: args{ - promoPaths: []string{"path1/component/path", "path3/component/also"}, - targetPaths: []string{"path1", "path2", "path3"}, + want: map[string]bool{ + "targetPath3": true, }, - want: []string{"path1", "path3"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := identifyCommonPaths(tt.args.promoPaths, tt.args.targetPaths) - assert.Equal(t, got, tt.want) + got := getPromotionSkipPaths(tt.args.promotion) + assert.Equal(t, tt.want, got) }) } } diff --git a/internal/pkg/githubapi/testdata/pr_body_multi_component.golden.md b/internal/pkg/githubapi/testdata/pr_body_multi_component.golden.md new file mode 100644 index 00000000..5294cc7b --- /dev/null +++ b/internal/pkg/githubapi/testdata/pr_body_multi_component.golden.md @@ -0,0 +1,4 @@ +↘️ #1 `sourcePath1` ➡️ +    `targetPath1` +    ↘️ #2 `sourcePath2` ➡️ +        `targetPath2`