From ab06623e1c16d98f5636e7f56c75a9e8d1ab98a8 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Mon, 21 Oct 2024 15:13:27 -0400 Subject: [PATCH 1/4] Improve promotion comment. Promotion comment used to list all the values in the 'targetPaths' key, ignoring the promotionAllow and promotionBlock lists. This PR limits the comment to only paths that are being promoted. --- internal/pkg/githubapi/github.go | 25 ++++++- internal/pkg/githubapi/github_test.go | 72 +++++++++++++++++++ .../pkg/githubapi/testdata/pr_body.golden.md | 1 - 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 29430cb3..8127136d 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -1109,7 +1109,7 @@ func prBody(keys []int, newPrMetadata prMetadata, newPrBody string) string { for i, k := range keys { sp = newPrMetadata.PreviousPromotionMetadata[k].SourcePath - x := newPrMetadata.PreviousPromotionMetadata[k].TargetPaths + x := identifyCommonPaths(newPrMetadata.PromotedPaths, newPrMetadata.PreviousPromotionMetadata[k].TargetPaths) 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) } @@ -1117,6 +1117,29 @@ 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 []string{} + } + commonPaths := []string{} + for _, pp := range promotionPaths { + 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) + } + } + } + + return commonPaths +} + func createPrObject(ghPrClientDetails GhPrClientDetails, newBranchRef string, newPrTitle string, newPrBody string, defaultBranch string, assignee string) (*github.PullRequest, error) { newPrConfig := &github.NewPullRequest{ Body: github.String(newPrBody), diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index 8284258b..670c7cf8 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "os" + "reflect" "testing" "time" @@ -235,6 +236,9 @@ func TestPrBody(t *testing.T) { t.Parallel() keys := []int{1, 2, 3} 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", "targetPath2", "targetPath4", "targetPath5", "targetPath6"}, PreviousPromotionMetadata: map[int]promotionInstanceMetaData{ 1: { SourcePath: "sourcePath1", @@ -338,3 +342,71 @@ func TestCommitStatusTargetURL(t *testing.T) { }) } } + +func Test_identifyCommonPaths(t *testing.T) { + type args struct { + promoPaths []string + targetPaths []string + } + tests := []struct { + name string + args args + want []string + }{ + { + 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", + args: args{ + promoPaths: []string{}, + targetPaths: []string{"path1", "path2", "path3"}, + }, + want: []string{}, + }, + { + name: "paths2 is empty", + args: args{ + promoPaths: []string{"path1/component/some", "path2/some/other", "path3"}, + targetPaths: []string{}, + }, + want: []string{}, + }, + { + name: "paths2 missing elements", + args: args{ + promoPaths: []string{"path1", "path2", "path3"}, + targetPaths: []string{""}, + }, + want: []string{}, + }, + { + name: "path1 missing elements", + args: args{ + promoPaths: []string{""}, + targetPaths: []string{"path1", "path2"}, + }, + want: []string{}, + }, + { + name: "path1 and path2 common elements", + args: args{ + promoPaths: []string{"path1/component/path", "path3/compoenet/also"}, + targetPaths: []string{"path1", "path2", "path3"}, + }, + want: []string{"path1", "path3"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := identifyCommonPaths(tt.args.promoPaths, tt.args.targetPaths); !reflect.DeepEqual(got, tt.want) { + t.Errorf("commonPaths() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/internal/pkg/githubapi/testdata/pr_body.golden.md b/internal/pkg/githubapi/testdata/pr_body.golden.md index ef3693fe..a01e4f2e 100644 --- a/internal/pkg/githubapi/testdata/pr_body.golden.md +++ b/internal/pkg/githubapi/testdata/pr_body.golden.md @@ -2,7 +2,6 @@     `targetPath1`     `targetPath2`     ↘️ #2 `sourcePath2` ➡️ -        `targetPath3`         `targetPath4`         ↘️ #3 `sourcePath3` ➡️             `targetPath5` From 0bd325bf1b2120ec47663bbee93ee5f41c2a9427 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Tue, 22 Oct 2024 09:14:57 -0400 Subject: [PATCH 2/4] Use testify library for assert --- internal/pkg/githubapi/github_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index 670c7cf8..e4b2c525 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "os" - "reflect" "testing" "time" @@ -404,9 +403,8 @@ func Test_identifyCommonPaths(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := identifyCommonPaths(tt.args.promoPaths, tt.args.targetPaths); !reflect.DeepEqual(got, tt.want) { - t.Errorf("commonPaths() = %v, want %v", got, tt.want) - } + got := identifyCommonPaths(tt.args.promoPaths, tt.args.targetPaths) + assert.Equal(t, got, tt.want) }) } } From 9e0487bb2189be2e364853cb155d2e216cdeb2b2 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Tue, 22 Oct 2024 11:17:03 -0400 Subject: [PATCH 3/4] return nil, use assert --- internal/pkg/githubapi/github.go | 7 +++++-- internal/pkg/githubapi/github_test.go | 10 +++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 8127136d..ec533e2d 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -1121,10 +1121,13 @@ func prBody(keys []int, newPrMetadata prMetadata, newPrBody string) string { // returns a slice containing paths in common. func identifyCommonPaths(promotionPaths []string, targetPaths []string) []string { if (len(promotionPaths) == 0) || (len(targetPaths) == 0) { - return []string{} + return nil } - commonPaths := []string{} + var commonPaths []string for _, pp := range promotionPaths { + if pp == "" { + continue + } for _, tp := range targetPaths { if tp == "" { continue diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index e4b2c525..d2bfee0d 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -366,7 +366,7 @@ func Test_identifyCommonPaths(t *testing.T) { promoPaths: []string{}, targetPaths: []string{"path1", "path2", "path3"}, }, - want: []string{}, + want: nil, }, { name: "paths2 is empty", @@ -374,7 +374,7 @@ func Test_identifyCommonPaths(t *testing.T) { promoPaths: []string{"path1/component/some", "path2/some/other", "path3"}, targetPaths: []string{}, }, - want: []string{}, + want: nil, }, { name: "paths2 missing elements", @@ -382,7 +382,7 @@ func Test_identifyCommonPaths(t *testing.T) { promoPaths: []string{"path1", "path2", "path3"}, targetPaths: []string{""}, }, - want: []string{}, + want: nil, }, { name: "path1 missing elements", @@ -390,12 +390,12 @@ func Test_identifyCommonPaths(t *testing.T) { promoPaths: []string{""}, targetPaths: []string{"path1", "path2"}, }, - want: []string{}, + want: nil, }, { name: "path1 and path2 common elements", args: args{ - promoPaths: []string{"path1/component/path", "path3/compoenet/also"}, + promoPaths: []string{"path1/component/path", "path3/component/also"}, targetPaths: []string{"path1", "path2", "path3"}, }, want: []string{"path1", "path3"}, From 5c71edd2c520efaad49658cda40f064df1cfe8f7 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Tue, 22 Oct 2024 13:19:11 -0400 Subject: [PATCH 4/4] Add t.Parallel() --- internal/pkg/githubapi/github_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go index d2bfee0d..1a5d40d8 100644 --- a/internal/pkg/githubapi/github_test.go +++ b/internal/pkg/githubapi/github_test.go @@ -343,6 +343,7 @@ func TestCommitStatusTargetURL(t *testing.T) { } func Test_identifyCommonPaths(t *testing.T) { + t.Parallel() type args struct { promoPaths []string targetPaths []string @@ -403,6 +404,7 @@ func Test_identifyCommonPaths(t *testing.T) { } 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) })