From 3e7a4c65fc237adf2d3096642f22ae49f3f6f317 Mon Sep 17 00:00:00 2001 From: Daniel Hiller Date: Mon, 11 Jul 2022 16:35:55 +0200 Subject: [PATCH] Exclude release branch configs Also use base and head for calculating the diff from a PR Signed-off-by: Daniel Hiller --- .../botreview/review/image_update.go | 9 ++- .../botreview/review/image_update_test.go | 73 +++++++++++++++++++ external-plugins/botreview/review/review.go | 18 +++-- robots/cmd/botreview/main.go | 1 + 4 files changed, 92 insertions(+), 9 deletions(-) diff --git a/external-plugins/botreview/review/image_update.go b/external-plugins/botreview/review/image_update.go index f514c47c294..75d28430227 100644 --- a/external-plugins/botreview/review/image_update.go +++ b/external-plugins/botreview/review/image_update.go @@ -38,10 +38,14 @@ These are the suspicious hunks I found: ` ) -var prowJobImageUpdateHunkBodyMatcher *regexp.Regexp +var ( + prowJobImageUpdateHunkBodyMatcher *regexp.Regexp + prowJobReleaseBranchFileNameMatcher *regexp.Regexp +) func init() { prowJobImageUpdateHunkBodyMatcher = regexp.MustCompile(`(?m)^(-[\s]+- image: [^\s]+$[\n]^\+[\s]+- image: [^\s]+|-[\s]+image: [^\s]+$[\n]^\+[\s]+image: [^\s]+)$`) + prowJobReleaseBranchFileNameMatcher = regexp.MustCompile(`.*\/[\w-]+-[0-9-\.]+\.yaml`) } type ProwJobImageUpdateResult struct { @@ -78,7 +82,8 @@ func (t *ProwJobImageUpdate) AddIfRelevant(fileDiff *diff.FileDiff) { // * who are not yaml if strings.TrimPrefix(fileDiff.OrigName, "a/") != fileName || !strings.HasSuffix(fileName, ".yaml") || - !strings.HasPrefix(fileName, "github/ci/prow-deploy/files/jobs") { + !strings.HasPrefix(fileName, "github/ci/prow-deploy/files/jobs") || + prowJobReleaseBranchFileNameMatcher.MatchString(fileName) { return } diff --git a/external-plugins/botreview/review/image_update_test.go b/external-plugins/botreview/review/image_update_test.go index 02720b52116..d851d1ab85e 100644 --- a/external-plugins/botreview/review/image_update_test.go +++ b/external-plugins/botreview/review/image_update_test.go @@ -87,3 +87,76 @@ func TestProwJobImageUpdate_Review(t1 *testing.T) { }) } } + +func TestProwJobImageUpdate_AddIfRelevant(t1 *testing.T) { + type fields struct { + relevantFileDiffs []*diff.FileDiff + notMatchingHunks []*diff.Hunk + } + type args struct { + fileDiff *diff.FileDiff + } + tests := []struct { + name string + fields fields + args args + expectedRelevantFileDiffs []*diff.FileDiff + }{ + { + name: "release branch config is ignored", + fields: fields{ + relevantFileDiffs: nil, + notMatchingHunks: nil, + }, + args: args{ + fileDiff: &diff.FileDiff{ + OrigName: "a/github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits-0.54.yaml", + OrigTime: nil, + NewName: "b/github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits-0.54.yaml", + NewTime: nil, + Extended: nil, + Hunks: nil, + }, + }, + }, + { + name: "non-release branch config is added", + fields: fields{ + relevantFileDiffs: nil, + notMatchingHunks: nil, + }, + args: args{ + fileDiff: &diff.FileDiff{ + OrigName: "a/github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml", + OrigTime: nil, + NewName: "b/github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml", + NewTime: nil, + Extended: nil, + Hunks: nil, + }, + }, + expectedRelevantFileDiffs: []*diff.FileDiff{ + { + OrigName: "a/github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml", + OrigTime: nil, + NewName: "b/github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml", + NewTime: nil, + Extended: nil, + Hunks: nil, + }, + }, + }, + } + for _, tt := range tests { + t1.Run(tt.name, func(t1 *testing.T) { + t := &ProwJobImageUpdate{ + relevantFileDiffs: tt.fields.relevantFileDiffs, + notMatchingHunks: tt.fields.notMatchingHunks, + } + t.AddIfRelevant(tt.args.fileDiff) + if !reflect.DeepEqual(tt.expectedRelevantFileDiffs, t.relevantFileDiffs) { + t1.Errorf("expectedRelevantFileDiffs not equal: %v\n, was\n%v", tt.expectedRelevantFileDiffs, t.relevantFileDiffs) + } + }) + } +} diff --git a/external-plugins/botreview/review/review.go b/external-plugins/botreview/review/review.go index 3336ac7ccd6..68b27ea92f3 100644 --- a/external-plugins/botreview/review/review.go +++ b/external-plugins/botreview/review/review.go @@ -71,13 +71,14 @@ func (n BasicResult) String() string { } type Reviewer struct { - l *logrus.Entry - org string - repo string - num int - user string - action github.PullRequestEventAction - dryRun bool + l *logrus.Entry + org string + repo string + num int + user string + action github.PullRequestEventAction + dryRun bool + BaseSHA string } func NewReviewer(l *logrus.Entry, action github.PullRequestEventAction, org string, repo string, num int, user string, dryRun bool) *Reviewer { @@ -113,6 +114,9 @@ func (r *Reviewer) ReviewLocalCode() ([]BotReviewResult, error) { r.info("preparing review") diffCommand := exec.Command("git", "diff", "..main") + if r.BaseSHA != "" { + diffCommand = exec.Command("git", "diff", fmt.Sprintf("%s..%s", r.BaseSHA, "HEAD")) + } output, err := diffCommand.Output() if err != nil { r.fatalF("could not fetch diff output: %v", err) diff --git a/robots/cmd/botreview/main.go b/robots/cmd/botreview/main.go index 51b95b094d4..c50bfda606d 100644 --- a/robots/cmd/botreview/main.go +++ b/robots/cmd/botreview/main.go @@ -120,6 +120,7 @@ func main() { // Perform review reviewer := review.NewReviewer(log, github.PullRequestActionEdited, o.org, o.repo, o.pullRequestNumber, user.Login, o.dryRun) + reviewer.BaseSHA = pullRequest.Base.SHA botReviewResults, err := reviewer.ReviewLocalCode() if err != nil { log.Info("no review results, cancelling review comments")