From 7c0ff88bca0ed0111c58f352c9762ae47cf98196 Mon Sep 17 00:00:00 2001 From: Amit Watve Date: Mon, 11 Mar 2019 18:07:02 -0700 Subject: [PATCH 01/10] Move common filtering logic from trigger to pjutil. --- prow/pjutil/BUILD.bazel | 2 + prow/pjutil/filter.go | 61 ++++++++ prow/pjutil/filter_test.go | 154 +++++++++++++++++++ prow/plugins/trigger/BUILD.bazel | 1 + prow/plugins/trigger/generic-comment.go | 56 ++----- prow/plugins/trigger/generic-comment_test.go | 134 +--------------- prow/plugins/trigger/pull-request.go | 3 +- 7 files changed, 232 insertions(+), 179 deletions(-) create mode 100644 prow/pjutil/filter.go create mode 100644 prow/pjutil/filter_test.go diff --git a/prow/pjutil/BUILD.bazel b/prow/pjutil/BUILD.bazel index 94d31ef2f994..c67e5d8edb61 100644 --- a/prow/pjutil/BUILD.bazel +++ b/prow/pjutil/BUILD.bazel @@ -11,6 +11,7 @@ load( go_library( name = "go_default_library", srcs = [ + "filter.go", "pjutil.go", "pprof.go", "tot.go", @@ -46,6 +47,7 @@ filegroup( go_test( name = "go_default_test", srcs = [ + "filter_test.go", "pjutil_test.go", "tot_test.go", ], diff --git a/prow/pjutil/filter.go b/prow/pjutil/filter.go new file mode 100644 index 000000000000..da9c1e9722e1 --- /dev/null +++ b/prow/pjutil/filter.go @@ -0,0 +1,61 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package pjutil + +import ( + "regexp" + + "k8s.io/test-infra/prow/config" +) + +var TestAllRe = regexp.MustCompile(`(?m)^/test all,?($|\s.*)`) + +// Filter digests a presubmit config to determine if: +// - we can be certain that the presubmit should run +// - we know that the presubmit is forced to run +// - what the default behavior should be if the presubmit +// runs conditionally and does not match trigger conditions +type Filter func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) + +// CommandFilter builds a filter for `/test foo` +func CommandFilter(body string) Filter { + return func(p config.Presubmit) (bool, bool, bool) { + return p.TriggerMatches(body), p.TriggerMatches(body), true + } +} + +// TestAllFilter builds a filter for the automatic behavior of `/test all`. +// Jobs that explicitly match `/test all` in their trigger regex will be +// handled by a commandFilter for the comment in question. +func TestAllFilter() Filter { + return func(p config.Presubmit) (bool, bool, bool) { + return !p.NeedsExplicitTrigger(), false, false + } +} + +// AggregateFilter builds a filter that evaluates the child filters in order +// and returns the first match +func AggregateFilter(filters []Filter) Filter { + return func(presubmit config.Presubmit) (bool, bool, bool) { + for _, filter := range filters { + if shouldRun, forced, defaults := filter(presubmit); shouldRun { + return shouldRun, forced, defaults + } + } + return false, false, false + } +} diff --git a/prow/pjutil/filter_test.go b/prow/pjutil/filter_test.go new file mode 100644 index 000000000000..eb74e850b5e6 --- /dev/null +++ b/prow/pjutil/filter_test.go @@ -0,0 +1,154 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package pjutil + +import ( + "testing" + + "k8s.io/test-infra/prow/config" +) + +func TestTestAllFilter(t *testing.T) { + var testCases = []struct { + name string + presubmits []config.Presubmit + expected [][]bool + }{ + { + name: "test all filter matches jobs which do not require human triggering", + presubmits: []config.Presubmit{ + { + JobBase: config.JobBase{ + Name: "always-runs", + }, + AlwaysRun: true, + }, + { + JobBase: config.JobBase{ + Name: "runs-if-changed", + }, + AlwaysRun: false, + RegexpChangeMatcher: config.RegexpChangeMatcher{ + RunIfChanged: "sometimes", + }, + }, + { + JobBase: config.JobBase{ + Name: "runs-if-triggered", + }, + Reporter: config.Reporter{ + Context: "runs-if-triggered", + }, + Trigger: `(?m)^/test (?:.*? )?trigger(?: .*?)?$`, + RerunCommand: "/test trigger", + }, + { + JobBase: config.JobBase{ + Name: "literal-test-all-trigger", + }, + Reporter: config.Reporter{ + Context: "runs-if-triggered", + }, + Trigger: `(?m)^/test (?:.*? )?all(?: .*?)?$`, + RerunCommand: "/test all", + }, + }, + expected: [][]bool{{true, false, false}, {true, false, false}, {false, false, false}, {false, false, false}}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + if len(testCase.presubmits) != len(testCase.expected) { + t.Fatalf("%s: have %d presubmits but only %d expected filter outputs", testCase.name, len(testCase.presubmits), len(testCase.expected)) + } + if err := config.SetPresubmitRegexes(testCase.presubmits); err != nil { + t.Fatalf("%s: could not set presubmit regexes: %v", testCase.name, err) + } + filter := TestAllFilter() + for i, presubmit := range testCase.presubmits { + actualFiltered, actualForced, actualDefault := filter(presubmit) + expectedFiltered, expectedForced, expectedDefault := testCase.expected[i][0], testCase.expected[i][1], testCase.expected[i][2] + if actualFiltered != expectedFiltered { + t.Errorf("%s: filter did not evaluate correctly, expected %v but got %v for %v", testCase.name, expectedFiltered, actualFiltered, presubmit.Name) + } + if actualForced != expectedForced { + t.Errorf("%s: filter did not determine forced correctly, expected %v but got %v for %v", testCase.name, expectedForced, actualForced, presubmit.Name) + } + if actualDefault != expectedDefault { + t.Errorf("%s: filter did not determine default correctly, expected %v but got %v for %v", testCase.name, expectedDefault, actualDefault, presubmit.Name) + } + } + }) + } +} + +func TestCommandFilter(t *testing.T) { + var testCases = []struct { + name string + body string + presubmits []config.Presubmit + expected [][]bool + }{ + { + name: "command filter matches jobs whose triggers match the body", + body: "/test trigger", + presubmits: []config.Presubmit{ + { + JobBase: config.JobBase{ + Name: "trigger", + }, + Trigger: `(?m)^/test (?:.*? )?trigger(?: .*?)?$`, + RerunCommand: "/test trigger", + }, + { + JobBase: config.JobBase{ + Name: "other-trigger", + }, + Trigger: `(?m)^/test (?:.*? )?other-trigger(?: .*?)?$`, + RerunCommand: "/test other-trigger", + }, + }, + expected: [][]bool{{true, true, true}, {false, false, true}}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + if len(testCase.presubmits) != len(testCase.expected) { + t.Fatalf("%s: have %d presubmits but only %d expected filter outputs", testCase.name, len(testCase.presubmits), len(testCase.expected)) + } + if err := config.SetPresubmitRegexes(testCase.presubmits); err != nil { + t.Fatalf("%s: could not set presubmit regexes: %v", testCase.name, err) + } + filter := CommandFilter(testCase.body) + for i, presubmit := range testCase.presubmits { + actualFiltered, actualForced, actualDefault := filter(presubmit) + expectedFiltered, expectedForced, expectedDefault := testCase.expected[i][0], testCase.expected[i][1], testCase.expected[i][2] + if actualFiltered != expectedFiltered { + t.Errorf("%s: filter did not evaluate correctly, expected %v but got %v for %v", testCase.name, expectedFiltered, actualFiltered, presubmit.Name) + } + if actualForced != expectedForced { + t.Errorf("%s: filter did not determine forced correctly, expected %v but got %v for %v", testCase.name, expectedForced, actualForced, presubmit.Name) + } + if actualDefault != expectedDefault { + t.Errorf("%s: filter did not determine default correctly, expected %v but got %v for %v", testCase.name, expectedDefault, actualDefault, presubmit.Name) + } + } + }) + } +} diff --git a/prow/plugins/trigger/BUILD.bazel b/prow/plugins/trigger/BUILD.bazel index 8559a2876140..54b269ad54eb 100644 --- a/prow/plugins/trigger/BUILD.bazel +++ b/prow/plugins/trigger/BUILD.bazel @@ -22,6 +22,7 @@ go_test( "//prow/github:go_default_library", "//prow/github/fakegithub:go_default_library", "//prow/labels:go_default_library", + "//prow/pjutil:go_default_library", "//prow/plugins:go_default_library", "//vendor/github.com/pkg/errors:go_default_library", "//vendor/github.com/sirupsen/logrus:go_default_library", diff --git a/prow/plugins/trigger/generic-comment.go b/prow/plugins/trigger/generic-comment.go index 37b53166bb0a..907df513c144 100644 --- a/prow/plugins/trigger/generic-comment.go +++ b/prow/plugins/trigger/generic-comment.go @@ -18,6 +18,7 @@ package trigger import ( "fmt" + "k8s.io/test-infra/prow/pjutil" "regexp" "github.com/sirupsen/logrus" @@ -29,7 +30,6 @@ import ( ) var okToTestRe = regexp.MustCompile(`(?m)^/ok-to-test\s*$`) -var testAllRe = regexp.MustCompile(`(?m)^/test all,?($|\s.*)`) var retestRe = regexp.MustCompile(`(?m)^/retest\s*$`) func handleGenericComment(c Client, trigger plugins.Trigger, gc github.GenericCommentEvent) error { @@ -44,7 +44,7 @@ func handleGenericComment(c Client, trigger plugins.Trigger, gc github.GenericCo return nil } // Skip comments not germane to this plugin - if !retestRe.MatchString(gc.Body) && !okToTestRe.MatchString(gc.Body) && !testAllRe.MatchString(gc.Body) { + if !retestRe.MatchString(gc.Body) && !okToTestRe.MatchString(gc.Body) && !pjutil.TestAllRe.MatchString(gc.Body) { matched := false for _, presubmit := range c.Config.Presubmits[gc.Repo.FullName] { matched = matched || presubmit.TriggerMatches(gc.Body) @@ -160,7 +160,7 @@ type changesGetter interface { // filterPresubmits determines which presubmits should run and which should be skipped // by evaluating the user-provided filter. -func filterPresubmits(filter filter, gitHubClient changesGetter, pr *github.PullRequest, presubmits []config.Presubmit, logger *logrus.Entry) ([]config.Presubmit, []config.Presubmit, error) { +func filterPresubmits(filter pjutil.Filter, gitHubClient changesGetter, pr *github.PullRequest, presubmits []config.Presubmit, logger *logrus.Entry) ([]config.Presubmit, []config.Presubmit, error) { org, repo, number, branch := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref changes := config.NewGitHubDeferredChangedFilesProvider(gitHubClient, org, repo, number) var toTrigger []config.Presubmit @@ -209,15 +209,15 @@ type statusGetter interface { GetCombinedStatus(org, repo, ref string) (*github.CombinedStatus, error) } -func presubmitFilter(honorOkToTest bool, statusGetter statusGetter, body, org, repo, sha string, logger *logrus.Entry) (filter, error) { +func presubmitFilter(honorOkToTest bool, statusGetter statusGetter, body, org, repo, sha string, logger *logrus.Entry) (pjutil.Filter, error) { // the filters determine if we should check whether a job should run, whether // it should run regardless of whether its triggering conditions match, and // what the default behavior should be for that check. Multiple filters // can match a single presubmit, so it is important to order them correctly // as they have precedence -- filters that override the false default should // match before others. We order filters by amount of specificity. - var filters []filter - filters = append(filters, commandFilter(body)) + var filters []pjutil.Filter + filters = append(filters, pjutil.CommandFilter(body)) if retestRe.MatchString(body) { logger.Debug("Using retest filter.") combinedStatus, err := statusGetter.GetCombinedStatus(org, repo, sha) @@ -235,52 +235,16 @@ func presubmitFilter(honorOkToTest bool, statusGetter statusGetter, body, org, r filters = append(filters, retestFilter(failedContexts, allContexts)) } - if (honorOkToTest && okToTestRe.MatchString(body)) || testAllRe.MatchString(body) { + if (honorOkToTest && okToTestRe.MatchString(body)) || pjutil.TestAllRe.MatchString(body) { logger.Debug("Using test-all filter.") - filters = append(filters, testAllFilter()) - } - return aggregateFilter(filters), nil -} - -// filter digests a presubmit config to determine if: -// - we can be certain that the presubmit should run -// - we know that the presubmit is forced to run -// - what the default behavior should be if the presubmit -// runs conditionally and does not match trigger conditions -type filter func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) - -// commandFilter builds a filter for `/test foo` -func commandFilter(body string) filter { - return func(p config.Presubmit) (bool, bool, bool) { - return p.TriggerMatches(body), p.TriggerMatches(body), true + filters = append(filters, pjutil.TestAllFilter()) } + return pjutil.AggregateFilter(filters), nil } // retestFilter builds a filter for `/retest` -func retestFilter(failedContexts, allContexts sets.String) filter { +func retestFilter(failedContexts, allContexts sets.String) pjutil.Filter { return func(p config.Presubmit) (bool, bool, bool) { return failedContexts.Has(p.Context) || (!p.NeedsExplicitTrigger() && !allContexts.Has(p.Context)), false, true } } - -// testAllFilter builds a filter for the automatic behavior of `/test all`. -// Jobs that explicitly match `/test all` in their trigger regex will be -// handled by a commandFilter for the comment in question. -func testAllFilter() filter { - return func(p config.Presubmit) (bool, bool, bool) { - return !p.NeedsExplicitTrigger(), false, false - } -} - -// aggregateFilter builds a filter that evaluates the child filters in order -// and returns the first match -func aggregateFilter(filters []filter) filter { - return func(presubmit config.Presubmit) (bool, bool, bool) { - for _, filter := range filters { - if shouldRun, forced, defaults := filter(presubmit); shouldRun { - return shouldRun, forced, defaults - } - } - return false, false, false - } -} diff --git a/prow/plugins/trigger/generic-comment_test.go b/prow/plugins/trigger/generic-comment_test.go index 46a74a600d1a..5d46aec8af3f 100644 --- a/prow/plugins/trigger/generic-comment_test.go +++ b/prow/plugins/trigger/generic-comment_test.go @@ -18,6 +18,7 @@ package trigger import ( "fmt" + "k8s.io/test-infra/prow/pjutil" "log" "reflect" "testing" @@ -1367,62 +1368,6 @@ func TestPresubmitFilter(t *testing.T) { } } -func TestCommandFilter(t *testing.T) { - var testCases = []struct { - name string - body string - presubmits []config.Presubmit - expected [][]bool - }{ - { - name: "command filter matches jobs whose triggers match the body", - body: "/test trigger", - presubmits: []config.Presubmit{ - { - JobBase: config.JobBase{ - Name: "trigger", - }, - Trigger: `(?m)^/test (?:.*? )?trigger(?: .*?)?$`, - RerunCommand: "/test trigger", - }, - { - JobBase: config.JobBase{ - Name: "other-trigger", - }, - Trigger: `(?m)^/test (?:.*? )?other-trigger(?: .*?)?$`, - RerunCommand: "/test other-trigger", - }, - }, - expected: [][]bool{{true, true, true}, {false, false, true}}, - }, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - if len(testCase.presubmits) != len(testCase.expected) { - t.Fatalf("%s: have %d presubmits but only %d expected filter outputs", testCase.name, len(testCase.presubmits), len(testCase.expected)) - } - if err := config.SetPresubmitRegexes(testCase.presubmits); err != nil { - t.Fatalf("%s: could not set presubmit regexes: %v", testCase.name, err) - } - filter := commandFilter(testCase.body) - for i, presubmit := range testCase.presubmits { - actualFiltered, actualForced, actualDefault := filter(presubmit) - expectedFiltered, expectedForced, expectedDefault := testCase.expected[i][0], testCase.expected[i][1], testCase.expected[i][2] - if actualFiltered != expectedFiltered { - t.Errorf("%s: filter did not evaluate correctly, expected %v but got %v for %v", testCase.name, expectedFiltered, actualFiltered, presubmit.Name) - } - if actualForced != expectedForced { - t.Errorf("%s: filter did not determine forced correctly, expected %v but got %v for %v", testCase.name, expectedForced, actualForced, presubmit.Name) - } - if actualDefault != expectedDefault { - t.Errorf("%s: filter did not determine default correctly, expected %v but got %v for %v", testCase.name, expectedDefault, actualDefault, presubmit.Name) - } - } - }) - } -} - func TestRetestFilter(t *testing.T) { var testCases = []struct { name string @@ -1508,81 +1453,6 @@ func TestRetestFilter(t *testing.T) { } } -func TestTestAllFilter(t *testing.T) { - var testCases = []struct { - name string - presubmits []config.Presubmit - expected [][]bool - }{ - { - name: "test all filter matches jobs which do not require human triggering", - presubmits: []config.Presubmit{ - { - JobBase: config.JobBase{ - Name: "always-runs", - }, - AlwaysRun: true, - }, - { - JobBase: config.JobBase{ - Name: "runs-if-changed", - }, - AlwaysRun: false, - RegexpChangeMatcher: config.RegexpChangeMatcher{ - RunIfChanged: "sometimes", - }, - }, - { - JobBase: config.JobBase{ - Name: "runs-if-triggered", - }, - Reporter: config.Reporter{ - Context: "runs-if-triggered", - }, - Trigger: `(?m)^/test (?:.*? )?trigger(?: .*?)?$`, - RerunCommand: "/test trigger", - }, - { - JobBase: config.JobBase{ - Name: "literal-test-all-trigger", - }, - Reporter: config.Reporter{ - Context: "runs-if-triggered", - }, - Trigger: `(?m)^/test (?:.*? )?all(?: .*?)?$`, - RerunCommand: "/test all", - }, - }, - expected: [][]bool{{true, false, false}, {true, false, false}, {false, false, false}, {false, false, false}}, - }, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - if len(testCase.presubmits) != len(testCase.expected) { - t.Fatalf("%s: have %d presubmits but only %d expected filter outputs", testCase.name, len(testCase.presubmits), len(testCase.expected)) - } - if err := config.SetPresubmitRegexes(testCase.presubmits); err != nil { - t.Fatalf("%s: could not set presubmit regexes: %v", testCase.name, err) - } - filter := testAllFilter() - for i, presubmit := range testCase.presubmits { - actualFiltered, actualForced, actualDefault := filter(presubmit) - expectedFiltered, expectedForced, expectedDefault := testCase.expected[i][0], testCase.expected[i][1], testCase.expected[i][2] - if actualFiltered != expectedFiltered { - t.Errorf("%s: filter did not evaluate correctly, expected %v but got %v for %v", testCase.name, expectedFiltered, actualFiltered, presubmit.Name) - } - if actualForced != expectedForced { - t.Errorf("%s: filter did not determine forced correctly, expected %v but got %v for %v", testCase.name, expectedForced, actualForced, presubmit.Name) - } - if actualDefault != expectedDefault { - t.Errorf("%s: filter did not determine default correctly, expected %v but got %v for %v", testCase.name, expectedDefault, actualDefault, presubmit.Name) - } - } - }) - } -} - func TestDetermineSkippedPresubmits(t *testing.T) { var testCases = []struct { name string @@ -1643,7 +1513,7 @@ func (c *fakeChangesGetter) GetPullRequestChanges(org, repo string, number int) func TestFilterPresubmits(t *testing.T) { var testCases = []struct { name string - filter filter + filter pjutil.Filter presubmits []config.Presubmit changesError bool expectedToTrigger, expectedToSkip []config.Presubmit diff --git a/prow/plugins/trigger/pull-request.go b/prow/plugins/trigger/pull-request.go index 1bff98cfe82e..f55f55b8d0f1 100644 --- a/prow/plugins/trigger/pull-request.go +++ b/prow/plugins/trigger/pull-request.go @@ -19,6 +19,7 @@ package trigger import ( "encoding/json" "fmt" + "k8s.io/test-infra/prow/pjutil" "net/url" "k8s.io/test-infra/prow/errorutil" @@ -218,7 +219,7 @@ func TrustedPullRequest(ghc githubClient, trigger plugins.Trigger, author, org, // buildAll ensures that all builds that should run and will be required are built func buildAll(c Client, pr *github.PullRequest, eventGUID string, elideSkippedContexts bool) error { - toTest, toSkip, err := filterPresubmits(testAllFilter(), c.GitHubClient, pr, c.Config.Presubmits[pr.Base.Repo.FullName], c.Logger) + toTest, toSkip, err := filterPresubmits(pjutil.TestAllFilter(), c.GitHubClient, pr, c.Config.Presubmits[pr.Base.Repo.FullName], c.Logger) if err != nil { return err } From bc49795443cebd3418f1f9a38cc9a5d42a0c751f Mon Sep 17 00:00:00 2001 From: Amit Watve Date: Mon, 11 Mar 2019 18:07:02 -0700 Subject: [PATCH 02/10] Update gerrit client to detect new messages on a change. --- prow/gerrit/client/client.go | 21 +++++++++++++++++++-- prow/gerrit/client/client_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/prow/gerrit/client/client.go b/prow/gerrit/client/client.go index 914345d9bdce..3c7e5e53670f 100644 --- a/prow/gerrit/client/client.go +++ b/prow/gerrit/client/client.go @@ -259,7 +259,7 @@ func (h *gerritInstanceHandler) queryChangesForProject(project string, lastUpdat opt := &gerrit.QueryChangeOptions{} opt.Query = append(opt.Query, "project:"+project) - opt.AdditionalFields = []string{"CURRENT_REVISION", "CURRENT_COMMIT", "CURRENT_FILES"} + opt.AdditionalFields = []string{"CURRENT_REVISION", "CURRENT_COMMIT", "CURRENT_FILES", "MESSAGES"} start := 0 @@ -324,7 +324,24 @@ func (h *gerritInstanceHandler) queryChangesForProject(project string, lastUpdat continue } - if created.Before(lastUpdate) { + changeMessages := change.Messages + newMessages := false + + for _, message := range changeMessages { + if message.RevisionNumber == rev.Number { + messageTime, err := time.Parse(layout, message.Date) + if err != nil { + logrus.WithError(err).Errorf("Parse time %v failed", message.Date) + continue + } + if messageTime.After(lastUpdate) { + newMessages = true + break + } + } + } + + if !newMessages && created.Before(lastUpdate) { // stale commit logrus.Infof("Change %d, latest revision updated %s before lastUpdate %s, skipping this patchset", change.Number, created, lastUpdate) continue diff --git a/prow/gerrit/client/client_test.go b/prow/gerrit/client/client_test.go index 1edfb6bbf16a..4f5622d4b253 100644 --- a/prow/gerrit/client/client_test.go +++ b/prow/gerrit/client/client_test.go @@ -99,6 +99,35 @@ func TestQueryChange(t *testing.T) { }, revisions: map[string][]string{}, }, + { + name: "one outdated change, but there's a new message", + lastUpdate: now, + changes: map[string][]gerrit.ChangeInfo{ + "foo": { + { + Project: "bar", + ID: "100", + CurrentRevision: "1-1", + Updated: now.Add(time.Hour).Format(layout), + Revisions: map[string]gerrit.RevisionInfo{ + "1-1": { + Created: now.Add(-time.Hour).Format(layout), + Number: 1, + }, + }, + Status: "NEW", + Messages: []gerrit.ChangeMessageInfo{ + { + Date: now.Add(time.Hour).Format(layout), + Message: "some message", + RevisionNumber: 1, + }, + }, + }, + }, + }, + revisions: map[string][]string{"foo": {"1-1"}}, + }, { name: "one up-to-date change", lastUpdate: now.Add(-time.Minute), From c9ec9a8c84731ec9882e9158fc3cbe06e025972f Mon Sep 17 00:00:00 2001 From: Amit Watve Date: Mon, 11 Mar 2019 18:07:02 -0700 Subject: [PATCH 03/10] Add filter based trigger logic to gerrit. Update adapter to use presubmit filters. --- prow/gerrit/adapter/BUILD.bazel | 5 +- prow/gerrit/adapter/adapter.go | 31 +++++--- prow/gerrit/adapter/adapter_test.go | 119 +++++++++++++++++++++++++--- prow/gerrit/adapter/trigger.go | 107 +++++++++++++++++++++++++ 4 files changed, 239 insertions(+), 23 deletions(-) create mode 100644 prow/gerrit/adapter/trigger.go diff --git a/prow/gerrit/adapter/BUILD.bazel b/prow/gerrit/adapter/BUILD.bazel index 88ac22005097..bd1342a92e38 100644 --- a/prow/gerrit/adapter/BUILD.bazel +++ b/prow/gerrit/adapter/BUILD.bazel @@ -2,7 +2,10 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", - srcs = ["adapter.go"], + srcs = [ + "adapter.go", + "trigger.go", + ], importpath = "k8s.io/test-infra/prow/gerrit/adapter", visibility = ["//visibility:public"], deps = [ diff --git a/prow/gerrit/adapter/adapter.go b/prow/gerrit/adapter/adapter.go index 89cd15cf3181..02e75ff91fde 100644 --- a/prow/gerrit/adapter/adapter.go +++ b/prow/gerrit/adapter/adapter.go @@ -278,15 +278,23 @@ func (c *Controller) ProcessChange(instance string, change client.ChangeInfo) er case client.New: presubmits := c.config().Presubmits[cloneURI.String()] presubmits = append(presubmits, c.config().Presubmits[cloneURI.Host+"/"+cloneURI.Path]...) - for _, presubmit := range presubmits { - if shouldRun, err := presubmit.ShouldRun(change.Branch, changedFiles, false, false); err != nil { - return fmt.Errorf("failed to determine if presubmit %q should run: %v", presubmit.Name, err) - } else if shouldRun { - jobSpecs = append(jobSpecs, jobSpec{ - spec: pjutil.PresubmitSpec(presubmit, refs), - labels: presubmit.Labels, - }) - } + var filters []pjutil.Filter + filter, err := messageFilter(c.lastUpdate, change, presubmits) + if err != nil { + logger.WithError(err).Warn("failed to create filter on messages for presubmits") + } else { + filters = append(filters, filter) + } + filters = append(filters, oldRevisionFilter(c.lastUpdate, change.Revisions[change.CurrentRevision])) + toTrigger, _, err := filterPresubmits(pjutil.AggregateFilter(filters), change, presubmits) + if err != nil { + return fmt.Errorf("failed to filter presubmits: %v", err) + } + for _, presubmit := range toTrigger { + jobSpecs = append(jobSpecs, jobSpec{ + spec: pjutil.PresubmitSpec(presubmit, refs), + labels: presubmit.Labels, + }) } } @@ -302,6 +310,11 @@ func (c *Controller) ProcessChange(instance string, change client.ChangeInfo) er } labels[client.GerritRevision] = change.CurrentRevision + gerritLabel, ok := labels[client.GerritReportLabel] + if !ok || gerritLabel == "" { + labels[client.GerritReportLabel] = client.CodeReview + } + pj := pjutil.NewProwJobWithAnnotation(jSpec.spec, labels, annotations) if _, err := c.kc.CreateProwJob(pj); err != nil { logger.WithError(err).Errorf("fail to create prowjob %v", pj) diff --git a/prow/gerrit/adapter/adapter_test.go b/prow/gerrit/adapter/adapter_test.go index 1e4a3da26efa..552db4f86cf3 100644 --- a/prow/gerrit/adapter/adapter_test.go +++ b/prow/gerrit/adapter/adapter_test.go @@ -21,7 +21,7 @@ import ( "testing" "time" - gerrit "github.com/andygrunwald/go-gerrit" + "github.com/andygrunwald/go-gerrit" "k8s.io/test-infra/prow/gerrit/client" @@ -31,6 +31,8 @@ import ( "k8s.io/test-infra/prow/config" ) +var timeNow = time.Date(1234, time.May, 15, 1, 2, 3, 4, time.UTC) + type fca struct { sync.Mutex c *config.Config @@ -195,7 +197,9 @@ func TestProcessChange(t *testing.T) { Project: "woof", Status: "NEW", Revisions: map[string]client.RevisionInfo{ - "1": {}, + "1": { + Created: timeNow.Format(layout), + }, }, }, }, @@ -207,7 +211,8 @@ func TestProcessChange(t *testing.T) { Status: "NEW", Revisions: map[string]client.RevisionInfo{ "1": { - Ref: "refs/changes/00/1/1", + Ref: "refs/changes/00/1/1", + Created: timeNow.Format(layout), }, }, }, @@ -222,10 +227,12 @@ func TestProcessChange(t *testing.T) { Status: "NEW", Revisions: map[string]client.RevisionInfo{ "1": { - Ref: "refs/changes/00/2/1", + Ref: "refs/changes/00/2/1", + Created: timeNow.Format(layout), }, "2": { - Ref: "refs/changes/00/2/2", + Ref: "refs/changes/00/2/2", + Created: timeNow.Format(layout), }, }, }, @@ -240,7 +247,8 @@ func TestProcessChange(t *testing.T) { Status: "NEW", Revisions: map[string]client.RevisionInfo{ "1": { - Ref: "refs/changes/00/1/1", + Ref: "refs/changes/00/1/1", + Created: timeNow.Format(layout), }, }, }, @@ -255,7 +263,8 @@ func TestProcessChange(t *testing.T) { Status: "MERGED", Revisions: map[string]client.RevisionInfo{ "1": { - Ref: "refs/changes/00/1/1", + Ref: "refs/changes/00/1/1", + Created: timeNow.Format(layout), }, }, }, @@ -270,7 +279,8 @@ func TestProcessChange(t *testing.T) { Status: "MERGED", Revisions: map[string]client.RevisionInfo{ "1": { - Ref: "refs/changes/00/1/1", + Ref: "refs/changes/00/1/1", + Created: timeNow.Format(layout), }, }, }, @@ -288,6 +298,7 @@ func TestProcessChange(t *testing.T) { "africa-lyrics.txt": {}, "important-code.go": {}, }, + Created: timeNow.Format(layout), }, }, }, @@ -306,6 +317,7 @@ func TestProcessChange(t *testing.T) { "README.md": {}, "let-it-go.txt": {}, }, + Created: timeNow.Format(layout), }, }, }, @@ -319,7 +331,9 @@ func TestProcessChange(t *testing.T) { Branch: "pony", Status: "NEW", Revisions: map[string]client.RevisionInfo{ - "1": {}, + "1": { + Created: timeNow.Format(layout), + }, }, }, numPJ: 3, @@ -332,7 +346,78 @@ func TestProcessChange(t *testing.T) { Branch: "baz", Status: "NEW", Revisions: map[string]client.RevisionInfo{ - "1": {}, + "1": { + Created: timeNow.Format(layout), + }, + }, + }, + numPJ: 1, + }, + { + name: "old presubmits don't run on old revision but trigger job does because new message", + change: client.ChangeInfo{ + CurrentRevision: "1", + Project: "test-infra", + Branch: "baz", + Status: "NEW", + Revisions: map[string]client.RevisionInfo{ + "1": { + Number: 1, + Created: timeNow.Add(-time.Hour).Format(layout), + }, + }, + Messages: []gerrit.ChangeMessageInfo{ + { + Message: "/test troll", + RevisionNumber: 1, + Date: timeNow.Add(time.Hour).Format(layout), + }, + }, + }, + numPJ: 1, + }, + { + name: "unrelated comment shouldn't trigger anything", + change: client.ChangeInfo{ + CurrentRevision: "1", + Project: "test-infra", + Branch: "baz", + Status: "NEW", + Revisions: map[string]client.RevisionInfo{ + "1": { + Number: 1, + Created: timeNow.Add(-time.Hour).Format(layout), + }, + }, + Messages: []gerrit.ChangeMessageInfo{ + { + Message: "/test diasghdgasudhkashdk", + RevisionNumber: 1, + Date: timeNow.Add(time.Hour).Format(layout), + }, + }, + }, + numPJ: 0, + }, + { + name: "trigger always run job on test all even if revision is old", + change: client.ChangeInfo{ + CurrentRevision: "1", + Project: "test-infra", + Branch: "baz", + Status: "NEW", + Revisions: map[string]client.RevisionInfo{ + "1": { + Number: 1, + Created: timeNow.Add(-time.Hour).Format(layout), + }, + }, + Messages: []gerrit.ChangeMessageInfo{ + { + Message: "/test all", + RevisionNumber: 1, + Date: timeNow.Add(time.Hour).Format(layout), + }, }, }, numPJ: 1, @@ -373,6 +458,13 @@ func TestProcessChange(t *testing.T) { }, AlwaysRun: true, }, + { + JobBase: config.JobBase{ + Name: "trigger-regex-all-branches", + }, + Trigger: `.*/test\s*troll.*`, + RerunCommand: "/test troll", + }, } if err := config.SetPresubmitRegexes(testInfraPresubmits); err != nil { t.Fatalf("could not set regexes: %v", err) @@ -408,9 +500,10 @@ func TestProcessChange(t *testing.T) { fkc := &fkc{} c := &Controller{ - config: fca.Config, - kc: fkc, - gc: &fgc{}, + config: fca.Config, + kc: fkc, + gc: &fgc{}, + lastUpdate: timeNow.Add(-time.Minute), } err := c.ProcessChange("https://gerrit", tc.change) diff --git a/prow/gerrit/adapter/trigger.go b/prow/gerrit/adapter/trigger.go new file mode 100644 index 000000000000..2166b7f3c2ee --- /dev/null +++ b/prow/gerrit/adapter/trigger.go @@ -0,0 +1,107 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package adapter + +import ( + "fmt" + "k8s.io/test-infra/prow/pjutil" + "time" + + "github.com/sirupsen/logrus" + + "k8s.io/test-infra/prow/config" + "k8s.io/test-infra/prow/gerrit/client" +) + +const layout = "2006-01-02 15:04:05" + +func filterPresubmits(filter pjutil.Filter, change client.ChangeInfo, presubmits []config.Presubmit) ([]config.Presubmit, []config.Presubmit, error) { + var toTrigger []config.Presubmit + var toSkip []config.Presubmit + + for _, presubmit := range presubmits { + matches, forced, defaults := filter(presubmit) + fmt.Printf("job name: %v, matches: %v\n", presubmit.Name, matches) + if !matches { + continue + } + + shouldRun, err := presubmit.ShouldRun(change.Branch, listChangedFiles(change), forced, defaults) + if err != nil { + return nil, nil, fmt.Errorf("failed to determine if presubmit %q should run: %v", presubmit.Name, err) + } + + if shouldRun { + toTrigger = append(toTrigger, presubmit) + } else { + toSkip = append(toSkip, presubmit) + } + } + logrus.WithFields(logrus.Fields{"gerrit change": change.Number, "to-trigger": toTrigger, "to-skip": toSkip}).Debugf("Filtered %d jobs, found %d to trigger and %d to skip.", len(presubmits), len(toTrigger), len(toSkip)) + return toTrigger, toSkip, nil +} + +// messageFilter builds a filter for jobs based on the messageBody matching the trigger regex of the jobs. +func messageFilter(lastUpdate time.Time, change client.ChangeInfo, presubmits []config.Presubmit) (pjutil.Filter, error) { + var filters []pjutil.Filter + currentRevision := change.Revisions[change.CurrentRevision].Number + for _, message := range change.Messages { + messageTime, err := time.Parse(layout, message.Date) + if err != nil { + logrus.WithError(err).Errorf("Parse time %v failed", message.Date) + continue + } + if message.RevisionNumber != currentRevision || messageTime.Before(lastUpdate) { + continue + + } + // Skip comments not germane to this plugin + if !pjutil.TestAllRe.MatchString(message.Message) { + matched := false + for _, presubmit := range presubmits { + matched = matched || presubmit.TriggerMatches(message.Message) + if matched { + filters = append(filters, pjutil.CommandFilter(message.Message)) + } + } + if !matched { + logrus.Infof("Comment %s doesn't match any triggering regex, skipping.", message.Message) + continue + } + + } else { + filters = append(filters, pjutil.TestAllFilter()) + } + + } + return pjutil.AggregateFilter(filters), nil +} + +// oldRevisionFilter builds a filter for jobs that have already been processed for a revision. +func oldRevisionFilter(lastUpdate time.Time, rev client.RevisionInfo) pjutil.Filter { + created, err := time.Parse(layout, rev.Created) + if err != nil { + logrus.WithError(err).Errorf("Parse time %v failed", rev.Created) + return func(p config.Presubmit) (bool, bool, bool) { + return false, false, false + } + } + + return func(p config.Presubmit) (bool, bool, bool) { + return created.After(lastUpdate), false, false + } +} From 147af6a48019b5b38a00c459e19b8f09dd3642ac Mon Sep 17 00:00:00 2001 From: Amit Watve Date: Mon, 11 Mar 2019 18:07:02 -0700 Subject: [PATCH 04/10] Update reporter logic to aggregate on same label, since now there can be multiple triggered jobs on a revision. This also now makes it possible to have different labels for different jobs. --- prow/gerrit/reporter/reporter.go | 27 ++-- prow/gerrit/reporter/reporter_test.go | 200 +++++++++++++++++++++----- 2 files changed, 178 insertions(+), 49 deletions(-) diff --git a/prow/gerrit/reporter/reporter.go b/prow/gerrit/reporter/reporter.go index 1edff506357b..f8f93d0e2c54 100644 --- a/prow/gerrit/reporter/reporter.go +++ b/prow/gerrit/reporter/reporter.go @@ -82,8 +82,9 @@ func (c *Client) ShouldReport(pj *v1.ProwJob) bool { // Only report when all jobs of the same type on the same revision finished selector := labels.Set{ - client.GerritRevision: pj.ObjectMeta.Labels[client.GerritRevision], - kube.ProwJobTypeLabel: pj.ObjectMeta.Labels[kube.ProwJobTypeLabel], + client.GerritRevision: pj.ObjectMeta.Labels[client.GerritRevision], + kube.ProwJobTypeLabel: pj.ObjectMeta.Labels[kube.ProwJobTypeLabel], + client.GerritReportLabel: pj.ObjectMeta.Labels[client.GerritReportLabel], } pjs, err := c.lister.List(selector.AsSelector()) if err != nil { @@ -91,10 +92,10 @@ func (c *Client) ShouldReport(pj *v1.ProwJob) bool { return false } - for _, pj := range pjs { - if pj.Status.State == v1.TriggeredState || pj.Status.State == v1.PendingState { - // other jobs are still running on this revision, skip report - logrus.WithField("prowjob", pj.ObjectMeta.Name).Info("Other jobs are still running on this revision") + for _, pjob := range pjs { + if pjob.Status.State == v1.TriggeredState || pjob.Status.State == v1.PendingState { + // other jobs with same label are still running on this revision, skip report + logrus.WithField("prowjob", pjob.ObjectMeta.Name).Info("Other jobs with same label are still running on this revision") return false } } @@ -110,13 +111,15 @@ func (c *Client) Report(pj *v1.ProwJob) error { clientGerritID := client.GerritID clientGerritInstance := client.GerritInstance pjTypeLabel := kube.ProwJobTypeLabel + gerritReportLabel := client.GerritReportLabel // list all prowjobs in the patchset matching pj's type (pre- or post-submit) selector := labels.Set{ clientGerritRevision: pj.ObjectMeta.Labels[clientGerritRevision], pjTypeLabel: pj.ObjectMeta.Labels[pjTypeLabel], + gerritReportLabel: pj.ObjectMeta.Labels[gerritReportLabel], } - pjsOnRevision, err := c.lister.List(selector.AsSelector()) + pjsOnRevisionWithSameLabel, err := c.lister.List(selector.AsSelector()) if err != nil { logrus.WithError(err).Errorf("Cannot list prowjob with selector %v", selector) return err @@ -127,22 +130,22 @@ func (c *Client) Report(pj *v1.ProwJob) error { success := 0 message := "" - for _, pjOnRevision := range pjsOnRevision { - if pjOnRevision.Status.PrevReportStates[c.GetName()] == pjOnRevision.Status.State { + for _, pjOnRevisionWithSameLabel := range pjsOnRevisionWithSameLabel { + if pjOnRevisionWithSameLabel.Status.PrevReportStates[c.GetName()] == pjOnRevisionWithSameLabel.Status.State { logrus.Infof("Revision %s has been reported already", pj.ObjectMeta.Labels[clientGerritRevision]) return nil } - if pjOnRevision.Status.State == v1.AbortedState { + if pjOnRevisionWithSameLabel.Status.State == v1.AbortedState { continue } total++ - if pjOnRevision.Status.State == v1.SuccessState { + if pjOnRevisionWithSameLabel.Status.State == v1.SuccessState { success++ } - message = fmt.Sprintf("%s\nJob %s finished with %s -- URL: %s", message, pjOnRevision.Spec.Job, pjOnRevision.Status.State, pjOnRevision.Status.URL) + message = fmt.Sprintf("%s\nJob %s finished with %s -- URL: %s", message, pjOnRevisionWithSameLabel.Spec.Job, pjOnRevisionWithSameLabel.Status.State, pjOnRevisionWithSameLabel.Status.URL) } message = fmt.Sprintf("%d out of %d jobs passed!\n%s", success, total, message) diff --git a/prow/gerrit/reporter/reporter_test.go b/prow/gerrit/reporter/reporter_test.go index 87b6d3b7a28a..5d7ef3edeb1b 100644 --- a/prow/gerrit/reporter/reporter_test.go +++ b/prow/gerrit/reporter/reporter_test.go @@ -96,8 +96,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritInstance: "gerrit", @@ -113,8 +114,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - client.GerritID: "123-abc", - client.GerritInstance: "gerrit", + client.GerritID: "123-abc", + client.GerritInstance: "gerrit", + client.GerritReportLabel: "Code-Review", }, }, Status: v1.ProwJobStatus{ @@ -127,8 +129,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -144,8 +147,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -172,8 +176,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -227,8 +232,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -255,8 +261,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -284,8 +291,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -307,8 +315,9 @@ func TestReport(t *testing.T) { { ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "def", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "def", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-def", @@ -333,12 +342,13 @@ func TestReport(t *testing.T) { expectLabel: map[string]string{client.CodeReview: client.LGTM}, }, { - name: "2 jobs, one passed, other job unfinished, should not report", + name: "2 jobs, one passed, other job unfinished with same label, should not report", pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -360,8 +370,9 @@ func TestReport(t *testing.T) { { ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -386,8 +397,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -409,8 +421,9 @@ func TestReport(t *testing.T) { { ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -439,8 +452,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -462,8 +476,9 @@ func TestReport(t *testing.T) { { ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -492,8 +507,9 @@ func TestReport(t *testing.T) { pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -515,8 +531,9 @@ func TestReport(t *testing.T) { { ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -569,8 +586,9 @@ func TestReport(t *testing.T) { { ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - client.GerritRevision: "abc", - kube.ProwJobTypeLabel: "presubmit", + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "Code-Review", }, Annotations: map[string]string{ client.GerritID: "123-abc", @@ -593,6 +611,114 @@ func TestReport(t *testing.T) { reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, expectLabel: map[string]string{"postsubmit-label": client.LGTM}, }, + { + name: "2 jobs, both passed, different label, should report by itself", + pj: &v1.ProwJob{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "label-foo", + }, + Annotations: map[string]string{ + client.GerritID: "123-abc", + client.GerritInstance: "gerrit", + }, + }, + Status: v1.ProwJobStatus{ + State: v1.SuccessState, + URL: "guber/foo", + }, + Spec: v1.ProwJobSpec{ + Refs: &v1.Refs{ + Repo: "foo", + }, + Job: "ci-foo", + }, + }, + existingPJs: []*v1.ProwJob{ + { + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "label-bar", + }, + Annotations: map[string]string{ + client.GerritID: "123-abc", + client.GerritInstance: "gerrit", + }, + }, + Status: v1.ProwJobStatus{ + State: v1.SuccessState, + URL: "guber/bar", + }, + Spec: v1.ProwJobSpec{ + Refs: &v1.Refs{ + Repo: "bar", + }, + Job: "ci-bar", + }, + }, + }, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + expectLabel: map[string]string{"label-foo": client.LGTM}, + }, + { + name: "2 jobs, one success one pending, different label, should report by itself", + pj: &v1.ProwJob{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "label-foo", + }, + Annotations: map[string]string{ + client.GerritID: "123-abc", + client.GerritInstance: "gerrit", + }, + }, + Status: v1.ProwJobStatus{ + State: v1.SuccessState, + URL: "guber/foo", + }, + Spec: v1.ProwJobSpec{ + Refs: &v1.Refs{ + Repo: "foo", + }, + Job: "ci-foo", + }, + }, + existingPJs: []*v1.ProwJob{ + { + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "label-bar", + }, + Annotations: map[string]string{ + client.GerritID: "123-abc", + client.GerritInstance: "gerrit", + }, + }, + Status: v1.ProwJobStatus{ + State: v1.PendingState, + URL: "guber/bar", + }, + Spec: v1.ProwJobSpec{ + Refs: &v1.Refs{ + Repo: "bar", + }, + Job: "ci-bar", + }, + }, + }, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + expectLabel: map[string]string{"label-foo": client.LGTM}, + }, } for _, tc := range testcases { From 11ea8010d4da8c4523ee593c6b1daada9a484759 Mon Sep 17 00:00:00 2001 From: Amit Watve Date: Mon, 11 Mar 2019 18:07:02 -0700 Subject: [PATCH 05/10] Modify Reporter interface to support updation of job report status on aggregated report. --- prow/crier/controller.go | 42 ++++++++-------- prow/gerrit/reporter/reporter.go | 16 +++--- prow/gerrit/reporter/reporter_test.go | 59 ++++++++++++++++++++++- prow/github/reporter/reporter.go | 4 +- prow/pubsub/reporter/reporter.go | 10 ++-- prow/pubsub/subscriber/subscriber.go | 2 +- prow/pubsub/subscriber/subscriber_test.go | 4 +- 7 files changed, 97 insertions(+), 40 deletions(-) diff --git a/prow/crier/controller.go b/prow/crier/controller.go index 14393bc0d6fb..5c4502dd1b1e 100644 --- a/prow/crier/controller.go +++ b/prow/crier/controller.go @@ -39,7 +39,7 @@ import ( ) type reportClient interface { - Report(pj *v1.ProwJob) error + Report(pj *v1.ProwJob) ([]*v1.ProwJob, error) GetName() string ShouldReport(pj *v1.ProwJob) bool } @@ -240,37 +240,37 @@ func (c *Controller) processNextItem() bool { } logrus.WithField("prowjob", keyRaw).Infof("Will report state : %s", pj.Status.State) - - if err := c.reporter.Report(pj); err != nil { + pjs, err := c.reporter.Report(pj) + if err != nil { logrus.WithError(err).WithField("prowjob", keyRaw).Error("failed to report job") return c.retry(key, err) } logrus.WithField("prowjob", keyRaw).Info("Updated job, now will update pj") + for _, pjob := range pjs { + if err := c.updateReportState(pjob); err != nil { + logrus.WithError(err).WithField("prowjob", keyRaw).Error("failed to update report state") - if err := c.updateReportState(pj); err != nil { - logrus.WithError(err).WithField("prowjob", keyRaw).Error("failed to update report state") + // theoretically patch should not have this issue, but in case: + // it might be out-dated, try to re-fetch pj and try again - // theoretically patch should not have this issue, but in case: - // it might be out-dated, try to re-fetch pj and try again + updatedPJ, err := c.pjclientset.Prow().ProwJobs(pjob.Namespace).Get(pjob.Name, metav1.GetOptions{}) + if err != nil { + logrus.WithError(err).WithField("prowjob", keyRaw).Error("failed to get prowjob from apiserver") + c.queue.Forget(key) + return true + } - updatedPJ, err := c.pjclientset.Prow().ProwJobs(pj.Namespace).Get(pj.Name, metav1.GetOptions{}) - if err != nil { - logrus.WithError(err).WithField("prowjob", keyRaw).Error("failed to get prowjob from apiserver") - c.queue.Forget(key) - return true + if err := c.updateReportState(updatedPJ); err != nil { + // shrug + logrus.WithError(err).WithField("prowjob", keyRaw).Error("failed to update report state again, give up") + c.queue.Forget(key) + return true + } } - if err := c.updateReportState(updatedPJ); err != nil { - // shrug - logrus.WithError(err).WithField("prowjob", keyRaw).Error("failed to update report state again, give up") - c.queue.Forget(key) - return true - } + logrus.WithField("prowjob", keyRaw).Infof("Hunky Dory!, pj : %v, state : %s", pjob.Spec.Job, pjob.Status.State) } - - logrus.WithField("prowjob", keyRaw).Infof("Hunky Dory!, pj : %v, state : %s", pj.Spec.Job, pj.Status.State) - c.queue.Forget(key) return true } diff --git a/prow/gerrit/reporter/reporter.go b/prow/gerrit/reporter/reporter.go index f8f93d0e2c54..b5c6484ddaf5 100644 --- a/prow/gerrit/reporter/reporter.go +++ b/prow/gerrit/reporter/reporter.go @@ -104,7 +104,7 @@ func (c *Client) ShouldReport(pj *v1.ProwJob) bool { } // Report will send the current prowjob status as a gerrit review -func (c *Client) Report(pj *v1.ProwJob) error { +func (c *Client) Report(pj *v1.ProwJob) ([]*v1.ProwJob, error) { // If you are hitting here, which means the entire patchset has been finished :-) clientGerritRevision := client.GerritRevision @@ -122,24 +122,24 @@ func (c *Client) Report(pj *v1.ProwJob) error { pjsOnRevisionWithSameLabel, err := c.lister.List(selector.AsSelector()) if err != nil { logrus.WithError(err).Errorf("Cannot list prowjob with selector %v", selector) - return err + return nil, err } // generate an aggregated report: total := 0 success := 0 message := "" - + var toReportJobs []*v1.ProwJob for _, pjOnRevisionWithSameLabel := range pjsOnRevisionWithSameLabel { if pjOnRevisionWithSameLabel.Status.PrevReportStates[c.GetName()] == pjOnRevisionWithSameLabel.Status.State { - logrus.Infof("Revision %s has been reported already", pj.ObjectMeta.Labels[clientGerritRevision]) - return nil + logrus.Infof("Job %s has been reported already", pjOnRevisionWithSameLabel.Name) + continue } if pjOnRevisionWithSameLabel.Status.State == v1.AbortedState { continue } - + toReportJobs = append(toReportJobs, pjOnRevisionWithSameLabel) total++ if pjOnRevisionWithSameLabel.Status.State == v1.SuccessState { success++ @@ -173,10 +173,10 @@ func (c *Client) Report(pj *v1.ProwJob) error { message = fmt.Sprintf("[NOTICE]: Prow Bot cannot access %s label!\n%s", reportLabel, message) if err := c.gc.SetReview(gerritInstance, gerritID, gerritRevision, message, nil); err != nil { logrus.WithError(err).Errorf("fail to set plain review on change ID %s", gerritID) - return err + return nil, err } } logrus.Infof("Review Complete") - return nil + return toReportJobs, nil } diff --git a/prow/gerrit/reporter/reporter_test.go b/prow/gerrit/reporter/reporter_test.go index 5d7ef3edeb1b..68f2a4ad53e9 100644 --- a/prow/gerrit/reporter/reporter_test.go +++ b/prow/gerrit/reporter/reporter_test.go @@ -665,6 +665,63 @@ func TestReport(t *testing.T) { reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, expectLabel: map[string]string{"label-foo": client.LGTM}, }, + { + name: "2 runs of same job, one reported, should report by itself", + pj: &v1.ProwJob{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "label-foo", + }, + Annotations: map[string]string{ + client.GerritID: "123-abc", + client.GerritInstance: "gerrit", + }, + }, + Status: v1.ProwJobStatus{ + State: v1.SuccessState, + URL: "guber/foo", + }, + Spec: v1.ProwJobSpec{ + Refs: &v1.Refs{ + Repo: "foo", + }, + Job: "ci-foo", + }, + }, + existingPJs: []*v1.ProwJob{ + { + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "label-foo", + }, + Annotations: map[string]string{ + client.GerritID: "123-abc", + client.GerritInstance: "gerrit", + }, + }, + Status: v1.ProwJobStatus{ + PrevReportStates: map[string]v1.ProwJobState{ + "gerrit-reporter": v1.FailureState, + }, + State: v1.FailureState, + URL: "guber/foo", + }, + Spec: v1.ProwJobSpec{ + Refs: &v1.Refs{ + Repo: "foo", + }, + Job: "ci-foo", + }, + }, + }, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + expectLabel: map[string]string{"label-foo": client.LGTM}, + }, { name: "2 jobs, one success one pending, different label, should report by itself", pj: &v1.ProwJob{ @@ -740,7 +797,7 @@ func TestReport(t *testing.T) { continue } - err := reporter.Report(tc.pj) + _, err := reporter.Report(tc.pj) if err != nil { t.Errorf("test: %s: expect no error but got error %v", tc.name, err) } diff --git a/prow/github/reporter/reporter.go b/prow/github/reporter/reporter.go index 5a27f2066bbe..e30b47bf0d4a 100644 --- a/prow/github/reporter/reporter.go +++ b/prow/github/reporter/reporter.go @@ -72,7 +72,7 @@ func (c *Client) ShouldReport(pj *v1.ProwJob) bool { } // Report will report via reportlib -func (c *Client) Report(pj *v1.ProwJob) error { +func (c *Client) Report(pj *v1.ProwJob) ([]*v1.ProwJob, error) { // TODO(krzyzacy): ditch ReportTemplate, and we can drop reference to config.Getter - return report.Report(c.gc, c.config().Plank.ReportTemplate, *pj, c.config().GitHubReporter.JobTypesToReport) + return []*v1.ProwJob{pj}, report.Report(c.gc, c.config().Plank.ReportTemplate, *pj, c.config().GitHubReporter.JobTypesToReport) } diff --git a/prow/pubsub/reporter/reporter.go b/prow/pubsub/reporter/reporter.go index bd68e04f84e7..07ca1efaa794 100644 --- a/prow/pubsub/reporter/reporter.go +++ b/prow/pubsub/reporter/reporter.go @@ -89,20 +89,20 @@ func (c *Client) ShouldReport(pj *prowapi.ProwJob) bool { // Report takes a prowjob, and generate a pubsub ReportMessage and publish to specific Pub/Sub topic // based on Pub/Sub related labels if they exist in this prowjob -func (c *Client) Report(pj *prowapi.ProwJob) error { +func (c *Client) Report(pj *prowapi.ProwJob) ([]*prowapi.ProwJob, error) { message := c.generateMessageFromPJ(pj) ctx := context.Background() client, err := pubsub.NewClient(ctx, message.Project) if err != nil { - return fmt.Errorf("could not create pubsub Client: %v", err) + return nil, fmt.Errorf("could not create pubsub Client: %v", err) } topic := client.Topic(message.Topic) d, err := json.Marshal(message) if err != nil { - return fmt.Errorf("could not marshal pubsub report: %v", err) + return nil, fmt.Errorf("could not marshal pubsub report: %v", err) } res := topic.Publish(ctx, &pubsub.Message{ @@ -111,10 +111,10 @@ func (c *Client) Report(pj *prowapi.ProwJob) error { _, err = res.Get(ctx) if err != nil { - return fmt.Errorf("failed to publish pubsub message: %v", err) + return nil, fmt.Errorf("failed to publish pubsub message: %v", err) } - return nil + return []*prowapi.ProwJob{pj}, nil } func (c *Client) generateMessageFromPJ(pj *prowapi.ProwJob) *ReportMessage { diff --git a/prow/pubsub/subscriber/subscriber.go b/prow/pubsub/subscriber/subscriber.go index 6cb2c89aba65..e91aa6796b7c 100644 --- a/prow/pubsub/subscriber/subscriber.go +++ b/prow/pubsub/subscriber/subscriber.go @@ -91,7 +91,7 @@ type messageInterface interface { } type reportClient interface { - Report(pj *prowapi.ProwJob) error + Report(pj *prowapi.ProwJob) ([]*prowapi.ProwJob, error) ShouldReport(pj *prowapi.ProwJob) bool } diff --git a/prow/pubsub/subscriber/subscriber_test.go b/prow/pubsub/subscriber/subscriber_test.go index 35dd8a8fdd75..05b70ba5895d 100644 --- a/prow/pubsub/subscriber/subscriber_test.go +++ b/prow/pubsub/subscriber/subscriber_test.go @@ -99,9 +99,9 @@ type fakeReporter struct { reported bool } -func (r *fakeReporter) Report(pj *prowapi.ProwJob) error { +func (r *fakeReporter) Report(pj *prowapi.ProwJob) ([]*prowapi.ProwJob, error) { r.reported = true - return nil + return nil, nil } func (r *fakeReporter) ShouldReport(pj *prowapi.ProwJob) bool { From 6a65d5c1cc6449bfe39247edf21d9cd9953741a1 Mon Sep 17 00:00:00 2001 From: Amit Watve Date: Mon, 11 Mar 2019 18:07:02 -0700 Subject: [PATCH 06/10] Unify FilterPresubmit logic and move to pjutil. --- prow/gerrit/adapter/adapter.go | 2 +- prow/gerrit/adapter/trigger.go | 27 --- prow/pjutil/filter.go | 27 +++ prow/pjutil/filter_test.go | 159 +++++++++++++ prow/plugins/trigger/BUILD.bazel | 1 - prow/plugins/trigger/generic-comment.go | 34 +-- prow/plugins/trigger/generic-comment_test.go | 232 ------------------- prow/plugins/trigger/pull-request.go | 7 +- 8 files changed, 198 insertions(+), 291 deletions(-) diff --git a/prow/gerrit/adapter/adapter.go b/prow/gerrit/adapter/adapter.go index 02e75ff91fde..67bcf082c3d4 100644 --- a/prow/gerrit/adapter/adapter.go +++ b/prow/gerrit/adapter/adapter.go @@ -286,7 +286,7 @@ func (c *Controller) ProcessChange(instance string, change client.ChangeInfo) er filters = append(filters, filter) } filters = append(filters, oldRevisionFilter(c.lastUpdate, change.Revisions[change.CurrentRevision])) - toTrigger, _, err := filterPresubmits(pjutil.AggregateFilter(filters), change, presubmits) + toTrigger, _, err := pjutil.FilterPresubmits(pjutil.AggregateFilter(filters), listChangedFiles(change), change.Branch, presubmits, logger) if err != nil { return fmt.Errorf("failed to filter presubmits: %v", err) } diff --git a/prow/gerrit/adapter/trigger.go b/prow/gerrit/adapter/trigger.go index 2166b7f3c2ee..4a9e0596e54e 100644 --- a/prow/gerrit/adapter/trigger.go +++ b/prow/gerrit/adapter/trigger.go @@ -17,7 +17,6 @@ limitations under the License. package adapter import ( - "fmt" "k8s.io/test-infra/prow/pjutil" "time" @@ -29,32 +28,6 @@ import ( const layout = "2006-01-02 15:04:05" -func filterPresubmits(filter pjutil.Filter, change client.ChangeInfo, presubmits []config.Presubmit) ([]config.Presubmit, []config.Presubmit, error) { - var toTrigger []config.Presubmit - var toSkip []config.Presubmit - - for _, presubmit := range presubmits { - matches, forced, defaults := filter(presubmit) - fmt.Printf("job name: %v, matches: %v\n", presubmit.Name, matches) - if !matches { - continue - } - - shouldRun, err := presubmit.ShouldRun(change.Branch, listChangedFiles(change), forced, defaults) - if err != nil { - return nil, nil, fmt.Errorf("failed to determine if presubmit %q should run: %v", presubmit.Name, err) - } - - if shouldRun { - toTrigger = append(toTrigger, presubmit) - } else { - toSkip = append(toSkip, presubmit) - } - } - logrus.WithFields(logrus.Fields{"gerrit change": change.Number, "to-trigger": toTrigger, "to-skip": toSkip}).Debugf("Filtered %d jobs, found %d to trigger and %d to skip.", len(presubmits), len(toTrigger), len(toSkip)) - return toTrigger, toSkip, nil -} - // messageFilter builds a filter for jobs based on the messageBody matching the trigger regex of the jobs. func messageFilter(lastUpdate time.Time, change client.ChangeInfo, presubmits []config.Presubmit) (pjutil.Filter, error) { var filters []pjutil.Filter diff --git a/prow/pjutil/filter.go b/prow/pjutil/filter.go index da9c1e9722e1..deb07b77139b 100644 --- a/prow/pjutil/filter.go +++ b/prow/pjutil/filter.go @@ -19,6 +19,7 @@ package pjutil import ( "regexp" + "github.com/sirupsen/logrus" "k8s.io/test-infra/prow/config" ) @@ -59,3 +60,29 @@ func AggregateFilter(filters []Filter) Filter { return false, false, false } } + +// FilterPresubmits determines which presubmits should run and which should be skipped +// by evaluating the user-provided filter. +func FilterPresubmits(filter Filter, changes config.ChangedFilesProvider, branch string, presubmits []config.Presubmit, logger *logrus.Entry) ([]config.Presubmit, []config.Presubmit, error) { + + var toTrigger []config.Presubmit + var toSkip []config.Presubmit + for _, presubmit := range presubmits { + matches, forced, defaults := filter(presubmit) + if !matches { + continue + } + shouldRun, err := presubmit.ShouldRun(branch, changes, forced, defaults) + if err != nil { + return nil, nil, err + } + if shouldRun { + toTrigger = append(toTrigger, presubmit) + } else { + toSkip = append(toSkip, presubmit) + } + } + + logger.WithFields(logrus.Fields{"to-trigger": toTrigger, "to-skip": toSkip}).Debugf("Filtered %d jobs, found %d to trigger and %d to skip.", len(presubmits), len(toTrigger), len(toSkip)) + return toTrigger, toSkip, nil +} diff --git a/prow/pjutil/filter_test.go b/prow/pjutil/filter_test.go index eb74e850b5e6..4c842124cc8a 100644 --- a/prow/pjutil/filter_test.go +++ b/prow/pjutil/filter_test.go @@ -17,8 +17,13 @@ limitations under the License. package pjutil import ( + "errors" + "reflect" "testing" + "github.com/sirupsen/logrus" + + "k8s.io/apimachinery/pkg/util/diff" "k8s.io/test-infra/prow/config" ) @@ -152,3 +157,157 @@ func TestCommandFilter(t *testing.T) { }) } } + +func fakeChangedFilesProvider(shouldError bool) config.ChangedFilesProvider { + return func() ([]string, error) { + if shouldError { + return nil, errors.New("error getting changes") + } + return nil, nil + } +} + +func TestFilterPresubmits(t *testing.T) { + var testCases = []struct { + name string + filter Filter + presubmits []config.Presubmit + changesError bool + expectedToTrigger, expectedToSkip []config.Presubmit + expectErr bool + }{ + { + name: "nothing matches, nothing to run or skip", + filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { + return false, false, false + }, + presubmits: []config.Presubmit{{ + JobBase: config.JobBase{Name: "ignored"}, + Reporter: config.Reporter{Context: "first"}, + }, { + JobBase: config.JobBase{Name: "ignored"}, + Reporter: config.Reporter{Context: "second"}, + }}, + changesError: false, + expectedToTrigger: nil, + expectedToSkip: nil, + expectErr: false, + }, + { + name: "everything matches and is forced to run, nothing to skip", + filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { + return true, true, true + }, + presubmits: []config.Presubmit{{ + JobBase: config.JobBase{Name: "should-trigger"}, + Reporter: config.Reporter{Context: "first"}, + }, { + JobBase: config.JobBase{Name: "should-trigger"}, + Reporter: config.Reporter{Context: "second"}, + }}, + changesError: false, + expectedToTrigger: []config.Presubmit{{ + JobBase: config.JobBase{Name: "should-trigger"}, + Reporter: config.Reporter{Context: "first"}, + }, { + JobBase: config.JobBase{Name: "should-trigger"}, + Reporter: config.Reporter{Context: "second"}, + }}, + expectedToSkip: nil, + expectErr: false, + }, + { + name: "error detecting if something should run, nothing to run or skip", + filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { + return true, false, false + }, + presubmits: []config.Presubmit{{ + JobBase: config.JobBase{Name: "errors"}, + Reporter: config.Reporter{Context: "first"}, + RegexpChangeMatcher: config.RegexpChangeMatcher{RunIfChanged: "oopsie"}, + }, { + JobBase: config.JobBase{Name: "ignored"}, + Reporter: config.Reporter{Context: "second"}, + }}, + changesError: true, + expectedToTrigger: nil, + expectedToSkip: nil, + expectErr: true, + }, + { + name: "some things match and are forced to run, nothing to skip", + filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { + return p.Name == "should-trigger", true, true + }, + presubmits: []config.Presubmit{{ + JobBase: config.JobBase{Name: "should-trigger"}, + Reporter: config.Reporter{Context: "first"}, + }, { + JobBase: config.JobBase{Name: "ignored"}, + Reporter: config.Reporter{Context: "second"}, + }}, + changesError: false, + expectedToTrigger: []config.Presubmit{{ + JobBase: config.JobBase{Name: "should-trigger"}, + Reporter: config.Reporter{Context: "first"}, + }}, + expectedToSkip: nil, + expectErr: false, + }, + { + name: "everything matches and some things are forced to run, others should be skipped", + filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { + return true, p.Name == "should-trigger", p.Name == "should-trigger" + }, + presubmits: []config.Presubmit{{ + JobBase: config.JobBase{Name: "should-trigger"}, + Reporter: config.Reporter{Context: "first"}, + }, { + JobBase: config.JobBase{Name: "should-trigger"}, + Reporter: config.Reporter{Context: "second"}, + }, { + JobBase: config.JobBase{Name: "should-skip"}, + Reporter: config.Reporter{Context: "third"}, + }, { + JobBase: config.JobBase{Name: "should-skip"}, + Reporter: config.Reporter{Context: "fourth"}, + }}, + changesError: false, + expectedToTrigger: []config.Presubmit{{ + JobBase: config.JobBase{Name: "should-trigger"}, + Reporter: config.Reporter{Context: "first"}, + }, { + JobBase: config.JobBase{Name: "should-trigger"}, + Reporter: config.Reporter{Context: "second"}, + }}, + expectedToSkip: []config.Presubmit{{ + JobBase: config.JobBase{Name: "should-skip"}, + Reporter: config.Reporter{Context: "third"}, + }, { + JobBase: config.JobBase{Name: "should-skip"}, + Reporter: config.Reporter{Context: "fourth"}, + }}, + expectErr: false, + }, + } + + branch := "foobar" + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + actualToTrigger, actualToSkip, err := FilterPresubmits(testCase.filter, fakeChangedFilesProvider(testCase.changesError), branch, testCase.presubmits, logrus.WithField("test-case", testCase.name)) + if testCase.expectErr && err == nil { + t.Errorf("%s: expected an error filtering presubmits, but got none", testCase.name) + } + if !testCase.expectErr && err != nil { + t.Errorf("%s: expected no error filtering presubmits, but got one: %v", testCase.name, err) + } + if !reflect.DeepEqual(actualToTrigger, testCase.expectedToTrigger) { + t.Errorf("%s: incorrect set of presubmits to skip: %s", testCase.name, diff.ObjectReflectDiff(actualToTrigger, testCase.expectedToTrigger)) + } + if !reflect.DeepEqual(actualToSkip, testCase.expectedToSkip) { + t.Errorf("%s: incorrect set of presubmits to skip: %s", testCase.name, diff.ObjectReflectDiff(actualToSkip, testCase.expectedToSkip)) + } + }) + } +} diff --git a/prow/plugins/trigger/BUILD.bazel b/prow/plugins/trigger/BUILD.bazel index 54b269ad54eb..8559a2876140 100644 --- a/prow/plugins/trigger/BUILD.bazel +++ b/prow/plugins/trigger/BUILD.bazel @@ -22,7 +22,6 @@ go_test( "//prow/github:go_default_library", "//prow/github/fakegithub:go_default_library", "//prow/labels:go_default_library", - "//prow/pjutil:go_default_library", "//prow/plugins:go_default_library", "//vendor/github.com/pkg/errors:go_default_library", "//vendor/github.com/sirupsen/logrus:go_default_library", diff --git a/prow/plugins/trigger/generic-comment.go b/prow/plugins/trigger/generic-comment.go index 907df513c144..bd140d458269 100644 --- a/prow/plugins/trigger/generic-comment.go +++ b/prow/plugins/trigger/generic-comment.go @@ -151,38 +151,14 @@ func FilterPresubmits(honorOkToTest bool, gitHubClient GitHubClient, body string return nil, nil, err } - return filterPresubmits(filter, gitHubClient, pr, presubmits, logger) -} - -type changesGetter interface { - GetPullRequestChanges(org, repo string, number int) ([]github.PullRequestChange, error) -} - -// filterPresubmits determines which presubmits should run and which should be skipped -// by evaluating the user-provided filter. -func filterPresubmits(filter pjutil.Filter, gitHubClient changesGetter, pr *github.PullRequest, presubmits []config.Presubmit, logger *logrus.Entry) ([]config.Presubmit, []config.Presubmit, error) { - org, repo, number, branch := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref + number, branch := pr.Number, pr.Base.Ref changes := config.NewGitHubDeferredChangedFilesProvider(gitHubClient, org, repo, number) - var toTrigger []config.Presubmit - var toSkipSuperset []config.Presubmit - for _, presubmit := range presubmits { - matches, forced, defaults := filter(presubmit) - if !matches { - continue - } - shouldRun, err := presubmit.ShouldRun(branch, changes, forced, defaults) - if err != nil { - return nil, nil, err - } - if shouldRun { - toTrigger = append(toTrigger, presubmit) - } else { - toSkipSuperset = append(toSkipSuperset, presubmit) - } + toTrigger, toSkipSuperset, err := pjutil.FilterPresubmits(filter, changes, branch, presubmits, logger) + if err != nil { + return nil, nil, err } toSkip := determineSkippedPresubmits(toTrigger, toSkipSuperset, logger) - logger.WithFields(logrus.Fields{"to-trigger": toTrigger, "to-skip": toSkip}).Debugf("Filtered %d jobs, found %d to trigger and %d to skip.", len(presubmits), len(toTrigger), len(toSkip)) - return toTrigger, toSkip, nil + return toTrigger, toSkip, err } // determineSkippedPresubmits identifies the largest set of contexts we can actually diff --git a/prow/plugins/trigger/generic-comment_test.go b/prow/plugins/trigger/generic-comment_test.go index 5d46aec8af3f..305584f6843b 100644 --- a/prow/plugins/trigger/generic-comment_test.go +++ b/prow/plugins/trigger/generic-comment_test.go @@ -18,7 +18,6 @@ package trigger import ( "fmt" - "k8s.io/test-infra/prow/pjutil" "log" "reflect" "testing" @@ -1498,234 +1497,3 @@ func TestDetermineSkippedPresubmits(t *testing.T) { }) } } - -type fakeChangesGetter struct { - shouldError bool -} - -func (c *fakeChangesGetter) GetPullRequestChanges(org, repo string, number int) ([]github.PullRequestChange, error) { - if c.shouldError { - return nil, errors.New("error getting changes") - } - return nil, nil -} - -func TestFilterPresubmits(t *testing.T) { - var testCases = []struct { - name string - filter pjutil.Filter - presubmits []config.Presubmit - changesError bool - expectedToTrigger, expectedToSkip []config.Presubmit - expectErr bool - }{ - { - name: "nothing matches, nothing to run or skip", - filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { - return false, false, false - }, - presubmits: []config.Presubmit{{ - JobBase: config.JobBase{Name: "ignored"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "ignored"}, - Reporter: config.Reporter{Context: "second"}, - }}, - changesError: false, - expectedToTrigger: nil, - expectedToSkip: nil, - expectErr: false, - }, - { - name: "everything matches and is forced to run, nothing to skip", - filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { - return true, true, true - }, - presubmits: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "second"}, - }}, - changesError: false, - expectedToTrigger: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "second"}, - }}, - expectedToSkip: nil, - expectErr: false, - }, - { - name: "error detecting if something should run, nothing to run or skip", - filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { - return true, false, false - }, - presubmits: []config.Presubmit{{ - JobBase: config.JobBase{Name: "errors"}, - Reporter: config.Reporter{Context: "first"}, - RegexpChangeMatcher: config.RegexpChangeMatcher{RunIfChanged: "oopsie"}, - }, { - JobBase: config.JobBase{Name: "ignored"}, - Reporter: config.Reporter{Context: "second"}, - }}, - changesError: true, - expectedToTrigger: nil, - expectedToSkip: nil, - expectErr: true, - }, - { - name: "some things match and are forced to run, nothing to skip", - filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { - return p.Name == "should-trigger", true, true - }, - presubmits: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "ignored"}, - Reporter: config.Reporter{Context: "second"}, - }}, - changesError: false, - expectedToTrigger: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "first"}, - }}, - expectedToSkip: nil, - expectErr: false, - }, - { - name: "everything matches and some things are forced to run, others should be skipped", - filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { - return true, p.Name == "should-trigger", p.Name == "should-trigger" - }, - presubmits: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "second"}, - }, { - JobBase: config.JobBase{Name: "should-skip"}, - Reporter: config.Reporter{Context: "third"}, - }, { - JobBase: config.JobBase{Name: "should-skip"}, - Reporter: config.Reporter{Context: "fourth"}, - }}, - changesError: false, - expectedToTrigger: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "second"}, - }}, - expectedToSkip: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-skip"}, - Reporter: config.Reporter{Context: "third"}, - }, { - JobBase: config.JobBase{Name: "should-skip"}, - Reporter: config.Reporter{Context: "fourth"}, - }}, - expectErr: false, - }, - { - name: "everything matches and some things are forced to run, others should be skipped, overlapping contexts filtered out", - filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { - return true, p.Name == "should-trigger", p.Name == "should-trigger" - }, - presubmits: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "second"}, - }, { - JobBase: config.JobBase{Name: "should-skip"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "should-skip"}, - Reporter: config.Reporter{Context: "fourth"}, - }}, - changesError: false, - expectedToTrigger: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "second"}, - }}, - expectedToSkip: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-skip"}, - Reporter: config.Reporter{Context: "fourth"}, - }}, - expectErr: false, - }, - { - name: "everything matches and some things are forced to run, others should be skipped, overlapping contexts filtered out even if skipped comes first in the list", - filter: func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { - return true, p.Name == "should-trigger", p.Name == "should-trigger" - }, - presubmits: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-skip"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "second"}, - }, { - JobBase: config.JobBase{Name: "should-skip"}, - Reporter: config.Reporter{Context: "fourth"}, - }}, - changesError: false, - expectedToTrigger: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "first"}, - }, { - JobBase: config.JobBase{Name: "should-trigger"}, - Reporter: config.Reporter{Context: "second"}, - }}, - expectedToSkip: []config.Presubmit{{ - JobBase: config.JobBase{Name: "should-skip"}, - Reporter: config.Reporter{Context: "fourth"}, - }}, - expectErr: false, - }, - } - - pr := &github.PullRequest{ - Base: github.PullRequestBranch{ - Repo: github.Repo{ - Owner: github.User{ - Login: "org", - }, - Name: "repo", - }, - Ref: "foobar", - }, - Number: 1, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - actualToTrigger, actualToSkip, err := filterPresubmits(testCase.filter, &fakeChangesGetter{shouldError: testCase.changesError}, pr, testCase.presubmits, logrus.WithField("test-case", testCase.name)) - if testCase.expectErr && err == nil { - t.Errorf("%s: expected an error filtering presubmits, but got none", testCase.name) - } - if !testCase.expectErr && err != nil { - t.Errorf("%s: expected no error filtering presubmits, but got one: %v", testCase.name, err) - } - if !reflect.DeepEqual(actualToTrigger, testCase.expectedToTrigger) { - t.Errorf("%s: incorrect set of presubmits to skip: %s", testCase.name, diff.ObjectReflectDiff(actualToTrigger, testCase.expectedToTrigger)) - } - if !reflect.DeepEqual(actualToSkip, testCase.expectedToSkip) { - t.Errorf("%s: incorrect set of presubmits to skip: %s", testCase.name, diff.ObjectReflectDiff(actualToSkip, testCase.expectedToSkip)) - } - }) - } -} diff --git a/prow/plugins/trigger/pull-request.go b/prow/plugins/trigger/pull-request.go index f55f55b8d0f1..68232070b4fd 100644 --- a/prow/plugins/trigger/pull-request.go +++ b/prow/plugins/trigger/pull-request.go @@ -22,6 +22,7 @@ import ( "k8s.io/test-infra/prow/pjutil" "net/url" + "k8s.io/test-infra/prow/config" "k8s.io/test-infra/prow/errorutil" "k8s.io/test-infra/prow/github" "k8s.io/test-infra/prow/labels" @@ -219,9 +220,13 @@ func TrustedPullRequest(ghc githubClient, trigger plugins.Trigger, author, org, // buildAll ensures that all builds that should run and will be required are built func buildAll(c Client, pr *github.PullRequest, eventGUID string, elideSkippedContexts bool) error { - toTest, toSkip, err := filterPresubmits(pjutil.TestAllFilter(), c.GitHubClient, pr, c.Config.Presubmits[pr.Base.Repo.FullName], c.Logger) + org, repo, number, branch := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref + changes := config.NewGitHubDeferredChangedFilesProvider(c.GitHubClient, org, repo, number) + toTest, toSkipSuperset, err := pjutil.FilterPresubmits(pjutil.TestAllFilter(), changes, branch, c.Config.Presubmits[pr.Base.Repo.FullName], c.Logger) if err != nil { return err } + + toSkip := determineSkippedPresubmits(toTest, toSkipSuperset, c.Logger) return runAndSkipJobs(c, pr, toTest, toSkip, eventGUID, elideSkippedContexts) } From dea9935819d45ce83e9324d3b4ec97fe397695d5 Mon Sep 17 00:00:00 2001 From: Amit Watve Date: Mon, 11 Mar 2019 18:07:02 -0700 Subject: [PATCH 07/10] Fix reporting logic for retriggered job. Add a couple of tests. --- prow/gerrit/adapter/adapter.go | 3 +- prow/gerrit/client/client_test.go | 32 +++- prow/gerrit/reporter/reporter.go | 56 +++++-- prow/gerrit/reporter/reporter_test.go | 219 ++++++++++++++++++++------ 4 files changed, 237 insertions(+), 73 deletions(-) diff --git a/prow/gerrit/adapter/adapter.go b/prow/gerrit/adapter/adapter.go index 67bcf082c3d4..298439b9d9d0 100644 --- a/prow/gerrit/adapter/adapter.go +++ b/prow/gerrit/adapter/adapter.go @@ -310,8 +310,7 @@ func (c *Controller) ProcessChange(instance string, change client.ChangeInfo) er } labels[client.GerritRevision] = change.CurrentRevision - gerritLabel, ok := labels[client.GerritReportLabel] - if !ok || gerritLabel == "" { + if gerritLabel, ok := labels[client.GerritReportLabel]; !ok || gerritLabel == "" { labels[client.GerritReportLabel] = client.CodeReview } diff --git a/prow/gerrit/client/client_test.go b/prow/gerrit/client/client_test.go index 4f5622d4b253..6719bb1bd005 100644 --- a/prow/gerrit/client/client_test.go +++ b/prow/gerrit/client/client_test.go @@ -80,7 +80,7 @@ func TestQueryChange(t *testing.T) { }, { name: "one outdated change", - lastUpdate: now, + lastUpdate: now.Add(-time.Minute), changes: map[string][]gerrit.ChangeInfo{ "foo": { { @@ -101,14 +101,14 @@ func TestQueryChange(t *testing.T) { }, { name: "one outdated change, but there's a new message", - lastUpdate: now, + lastUpdate: now.Add(-time.Minute), changes: map[string][]gerrit.ChangeInfo{ "foo": { { Project: "bar", ID: "100", CurrentRevision: "1-1", - Updated: now.Add(time.Hour).Format(layout), + Updated: now.Format(layout), Revisions: map[string]gerrit.RevisionInfo{ "1-1": { Created: now.Add(-time.Hour).Format(layout), @@ -118,7 +118,7 @@ func TestQueryChange(t *testing.T) { Status: "NEW", Messages: []gerrit.ChangeMessageInfo{ { - Date: now.Add(time.Hour).Format(layout), + Date: now.Format(layout), Message: "some message", RevisionNumber: 1, }, @@ -451,6 +451,30 @@ func TestQueryChange(t *testing.T) { "foo": {"2-1"}, }, }, + { + name: "merged change with new message, should ignore", + lastUpdate: now.Add(-time.Minute), + changes: map[string][]gerrit.ChangeInfo{ + "foo": { + { + Project: "bar", + ID: "2", + CurrentRevision: "2-1", + Updated: now.Format(layout), + Submitted: now.Add(-time.Hour).Format(layout), + Status: "MERGED", + Messages: []gerrit.ChangeMessageInfo{ + { + Date: now.Format(layout), + Message: "some message", + RevisionNumber: 1, + }, + }, + }, + }, + }, + revisions: map[string][]string{}, + }, } for _, tc := range testcases { diff --git a/prow/gerrit/reporter/reporter.go b/prow/gerrit/reporter/reporter.go index b5c6484ddaf5..bb57f97836f8 100644 --- a/prow/gerrit/reporter/reporter.go +++ b/prow/gerrit/reporter/reporter.go @@ -82,10 +82,17 @@ func (c *Client) ShouldReport(pj *v1.ProwJob) bool { // Only report when all jobs of the same type on the same revision finished selector := labels.Set{ - client.GerritRevision: pj.ObjectMeta.Labels[client.GerritRevision], - kube.ProwJobTypeLabel: pj.ObjectMeta.Labels[kube.ProwJobTypeLabel], - client.GerritReportLabel: pj.ObjectMeta.Labels[client.GerritReportLabel], + client.GerritRevision: pj.ObjectMeta.Labels[client.GerritRevision], + kube.ProwJobTypeLabel: pj.ObjectMeta.Labels[kube.ProwJobTypeLabel], } + + if pj.ObjectMeta.Labels[client.GerritReportLabel] == "" { + // Shouldn't happen, adapter should already have defaulted to Code-Review + logrus.Errorf("Gerrit report label not set for job %s", pj.Spec.Job) + } else { + selector[client.GerritReportLabel] = pj.ObjectMeta.Labels[client.GerritReportLabel] + } + pjs, err := c.lister.List(selector.AsSelector()) if err != nil { logrus.WithError(err).Errorf("Cannot list prowjob with selector %v", selector) @@ -107,21 +114,30 @@ func (c *Client) ShouldReport(pj *v1.ProwJob) bool { func (c *Client) Report(pj *v1.ProwJob) ([]*v1.ProwJob, error) { // If you are hitting here, which means the entire patchset has been finished :-) + logger := logrus.WithField("prowjob", pj) + clientGerritRevision := client.GerritRevision clientGerritID := client.GerritID clientGerritInstance := client.GerritInstance pjTypeLabel := kube.ProwJobTypeLabel gerritReportLabel := client.GerritReportLabel - // list all prowjobs in the patchset matching pj's type (pre- or post-submit) selector := labels.Set{ clientGerritRevision: pj.ObjectMeta.Labels[clientGerritRevision], pjTypeLabel: pj.ObjectMeta.Labels[pjTypeLabel], - gerritReportLabel: pj.ObjectMeta.Labels[gerritReportLabel], } + if pj.ObjectMeta.Labels[gerritReportLabel] == "" { + // Shouldn't happen, adapter should already have defaulted to Code-Review + logger.Errorf("Gerrit report label not set for job %s", pj.Spec.Job) + } else { + selector[gerritReportLabel] = pj.ObjectMeta.Labels[gerritReportLabel] + } + + // list all prowjobs in the patchset matching pj's type (pre- or post-submit) + pjsOnRevisionWithSameLabel, err := c.lister.List(selector.AsSelector()) if err != nil { - logrus.WithError(err).Errorf("Cannot list prowjob with selector %v", selector) + logger.WithError(err).Errorf("Cannot list prowjob with selector %v", selector) return nil, err } @@ -130,12 +146,14 @@ func (c *Client) Report(pj *v1.ProwJob) ([]*v1.ProwJob, error) { success := 0 message := "" var toReportJobs []*v1.ProwJob + mostRecentJob := map[string]*v1.ProwJob{} for _, pjOnRevisionWithSameLabel := range pjsOnRevisionWithSameLabel { - if pjOnRevisionWithSameLabel.Status.PrevReportStates[c.GetName()] == pjOnRevisionWithSameLabel.Status.State { - logrus.Infof("Job %s has been reported already", pjOnRevisionWithSameLabel.Name) - continue + job, ok := mostRecentJob[pjOnRevisionWithSameLabel.Spec.Job] + if !ok || job.CreationTimestamp.Time.Before(pjOnRevisionWithSameLabel.CreationTimestamp.Time) { + mostRecentJob[pjOnRevisionWithSameLabel.Spec.Job] = pjOnRevisionWithSameLabel } - + } + for _, pjOnRevisionWithSameLabel := range mostRecentJob { if pjOnRevisionWithSameLabel.Status.State == v1.AbortedState { continue } @@ -148,6 +166,12 @@ func (c *Client) Report(pj *v1.ProwJob) ([]*v1.ProwJob, error) { message = fmt.Sprintf("%s\nJob %s finished with %s -- URL: %s", message, pjOnRevisionWithSameLabel.Spec.Job, pjOnRevisionWithSameLabel.Status.State, pjOnRevisionWithSameLabel.Status.URL) } + if total <= 0 { + // Shouldn't happen but return if does + logger.Warn("Tried to report empty or aborted jobs.") + return nil, nil + } + message = fmt.Sprintf("%d out of %d jobs passed!\n%s", success, total, message) // report back @@ -163,20 +187,20 @@ func (c *Client) Report(pj *v1.ProwJob) ([]*v1.ProwJob, error) { if success == total { vote = client.LGTM } - labels := map[string]string{reportLabel: vote} + reviewLabels := map[string]string{reportLabel: vote} - logrus.Infof("Reporting to instance %s on id %s with message %s", gerritInstance, gerritID, message) - if err := c.gc.SetReview(gerritInstance, gerritID, gerritRevision, message, labels); err != nil { - logrus.WithError(err).Errorf("fail to set review with %s label on change ID %s", reportLabel, gerritID) + logger.Infof("Reporting to instance %s on id %s with message %s", gerritInstance, gerritID, message) + if err := c.gc.SetReview(gerritInstance, gerritID, gerritRevision, message, reviewLabels); err != nil { + logger.WithError(err).Errorf("fail to set review with %s label on change ID %s", reportLabel, gerritID) // possibly don't have label permissions, try without labels message = fmt.Sprintf("[NOTICE]: Prow Bot cannot access %s label!\n%s", reportLabel, message) if err := c.gc.SetReview(gerritInstance, gerritID, gerritRevision, message, nil); err != nil { - logrus.WithError(err).Errorf("fail to set plain review on change ID %s", gerritID) + logger.WithError(err).Errorf("fail to set plain review on change ID %s", gerritID) return nil, err } } - logrus.Infof("Review Complete") + logger.Infof("Review Complete, reported jobs: %v", toReportJobs) return toReportJobs, nil } diff --git a/prow/gerrit/reporter/reporter_test.go b/prow/gerrit/reporter/reporter_test.go index 68f2a4ad53e9..a6890dfd1380 100644 --- a/prow/gerrit/reporter/reporter_test.go +++ b/prow/gerrit/reporter/reporter_test.go @@ -21,6 +21,7 @@ import ( "reflect" "strings" "testing" + "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -30,6 +31,8 @@ import ( "k8s.io/test-infra/prow/kube" ) +var timeNow = time.Date(1234, time.May, 15, 1, 2, 3, 4, time.UTC) + type fgc struct { reportMessage string reportLabel map[string]string @@ -65,15 +68,15 @@ func (fl fakeLister) ProwJobs(namespace string) pjlister.ProwJobNamespaceLister } func TestReport(t *testing.T) { - var testcases = []struct { - name string - pj *v1.ProwJob - existingPJs []*v1.ProwJob - expectReport bool - reportInclude []string - reportExclude []string - expectLabel map[string]string + name string + pj *v1.ProwJob + existingPJs []*v1.ProwJob + expectReport bool + reportInclude []string + reportExclude []string + expectLabel map[string]string + numExpectedReport int }{ { name: "1 job, unfinished, should not report", @@ -167,9 +170,10 @@ func TestReport(t *testing.T) { Job: "ci-foo", }, }, - expectReport: true, - reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, - expectLabel: map[string]string{client.CodeReview: client.LGTM}, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + expectLabel: map[string]string{client.CodeReview: client.LGTM}, + numExpectedReport: 1, }, { name: "1 job, aborted, should not report", @@ -223,9 +227,10 @@ func TestReport(t *testing.T) { Job: "ci-foo", }, }, - expectReport: true, - reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, - expectLabel: map[string]string{"foobar-label": client.LGTM}, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + expectLabel: map[string]string{"foobar-label": client.LGTM}, + numExpectedReport: 1, }, { name: "1 job, failed, should report", @@ -252,9 +257,10 @@ func TestReport(t *testing.T) { Job: "ci-foo", }, }, - expectReport: true, - reportInclude: []string{"0 out of 1", "ci-foo", "failure", "guber/foo"}, - expectLabel: map[string]string{client.CodeReview: client.LBTM}, + expectReport: true, + reportInclude: []string{"0 out of 1", "ci-foo", "failure", "guber/foo"}, + expectLabel: map[string]string{client.CodeReview: client.LBTM}, + numExpectedReport: 1, }, { name: "1 job, passed, has slash in repo name, should report and handle slash properly", @@ -281,10 +287,11 @@ func TestReport(t *testing.T) { Job: "ci-foo-bar", }, }, - expectReport: true, - reportInclude: []string{"1 out of 1", "ci-foo-bar", "success", "guber/foo/bar"}, - reportExclude: []string{"foo_bar"}, - expectLabel: map[string]string{client.CodeReview: client.LGTM}, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo-bar", "success", "guber/foo/bar"}, + reportExclude: []string{"foo_bar"}, + expectLabel: map[string]string{client.CodeReview: client.LGTM}, + numExpectedReport: 1, }, { name: "2 jobs, one passed, other job finished but on different revision, should report", @@ -336,10 +343,11 @@ func TestReport(t *testing.T) { }, }, }, - expectReport: true, - reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, - reportExclude: []string{"2", "bar"}, - expectLabel: map[string]string{client.CodeReview: client.LGTM}, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + reportExclude: []string{"2", "bar"}, + expectLabel: map[string]string{client.CodeReview: client.LGTM}, + numExpectedReport: 1, }, { name: "2 jobs, one passed, other job unfinished with same label, should not report", @@ -442,10 +450,11 @@ func TestReport(t *testing.T) { }, }, }, - expectReport: true, - reportInclude: []string{"1 out of 2", "ci-foo", "success", "ci-bar", "failure", "guber/foo", "guber/bar"}, - reportExclude: []string{"0", "2 out of 2"}, - expectLabel: map[string]string{client.CodeReview: client.LBTM}, + expectReport: true, + reportInclude: []string{"1 out of 2", "ci-foo", "success", "ci-bar", "failure", "guber/foo", "guber/bar"}, + reportExclude: []string{"0", "2 out of 2"}, + expectLabel: map[string]string{client.CodeReview: client.LBTM}, + numExpectedReport: 2, }, { name: "2 jobs, both passed, should report", @@ -497,10 +506,11 @@ func TestReport(t *testing.T) { }, }, }, - expectReport: true, - reportInclude: []string{"2 out of 2", "ci-foo", "success", "ci-bar", "guber/foo", "guber/bar"}, - reportExclude: []string{"1", "0", "failure"}, - expectLabel: map[string]string{client.CodeReview: client.LGTM}, + expectReport: true, + reportInclude: []string{"2 out of 2", "ci-foo", "success", "ci-bar", "guber/foo", "guber/bar"}, + reportExclude: []string{"1", "0", "failure"}, + expectLabel: map[string]string{client.CodeReview: client.LGTM}, + numExpectedReport: 2, }, { name: "2 jobs, one passed, one aborted, should report but skip aborted job", @@ -552,10 +562,11 @@ func TestReport(t *testing.T) { }, }, }, - expectReport: true, - reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, - reportExclude: []string{"2", "0", "failure", "aborted", "ci-bar", "guber/bar"}, - expectLabel: map[string]string{client.CodeReview: client.LGTM}, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + reportExclude: []string{"2", "0", "failure", "aborted", "ci-bar", "guber/bar"}, + expectLabel: map[string]string{client.CodeReview: client.LGTM}, + numExpectedReport: 1, }, { name: "postsubmit after presubmit on same revision, should report separately", @@ -607,9 +618,10 @@ func TestReport(t *testing.T) { }, }, }, - expectReport: true, - reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, - expectLabel: map[string]string{"postsubmit-label": client.LGTM}, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + expectLabel: map[string]string{"postsubmit-label": client.LGTM}, + numExpectedReport: 1, }, { name: "2 jobs, both passed, different label, should report by itself", @@ -661,12 +673,13 @@ func TestReport(t *testing.T) { }, }, }, - expectReport: true, - reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, - expectLabel: map[string]string{"label-foo": client.LGTM}, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + expectLabel: map[string]string{"label-foo": client.LGTM}, + numExpectedReport: 1, }, { - name: "2 runs of same job, one reported, should report by itself", + name: "one job, reported, retriggered, should report by itself", pj: &v1.ProwJob{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ @@ -678,6 +691,9 @@ func TestReport(t *testing.T) { client.GerritID: "123-abc", client.GerritInstance: "gerrit", }, + CreationTimestamp: metav1.Time{ + Time: timeNow, + }, }, Status: v1.ProwJobStatus{ State: v1.SuccessState, @@ -702,6 +718,9 @@ func TestReport(t *testing.T) { client.GerritID: "123-abc", client.GerritInstance: "gerrit", }, + CreationTimestamp: metav1.Time{ + Time: timeNow.Add(-time.Minute), + }, }, Status: v1.ProwJobStatus{ PrevReportStates: map[string]v1.ProwJobState{ @@ -718,9 +737,10 @@ func TestReport(t *testing.T) { }, }, }, - expectReport: true, - reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, - expectLabel: map[string]string{"label-foo": client.LGTM}, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + expectLabel: map[string]string{"label-foo": client.LGTM}, + numExpectedReport: 1, }, { name: "2 jobs, one success one pending, different label, should report by itself", @@ -772,9 +792,103 @@ func TestReport(t *testing.T) { }, }, }, - expectReport: true, - reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, - expectLabel: map[string]string{"label-foo": client.LGTM}, + expectReport: true, + reportInclude: []string{"1 out of 1", "ci-foo", "success", "guber/foo"}, + expectLabel: map[string]string{"label-foo": client.LGTM}, + numExpectedReport: 1, + }, + { + name: "2 jobs, both failed, already reported, same label, retrigger one and passed, should report both and not lgtm", + pj: &v1.ProwJob{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "same-label", + }, + Annotations: map[string]string{ + client.GerritID: "123-abc", + client.GerritInstance: "gerrit", + }, + CreationTimestamp: metav1.Time{ + Time: timeNow, + }, + }, + Status: v1.ProwJobStatus{ + State: v1.SuccessState, + URL: "guber/foo", + }, + Spec: v1.ProwJobSpec{ + Refs: &v1.Refs{ + Repo: "foo", + }, + Job: "ci-foo", + }, + }, + existingPJs: []*v1.ProwJob{ + { + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "same-label", + }, + Annotations: map[string]string{ + client.GerritID: "123-abc", + client.GerritInstance: "gerrit", + }, + CreationTimestamp: metav1.Time{ + Time: timeNow.Add(-time.Hour), + }, + }, + Status: v1.ProwJobStatus{ + State: v1.FailureState, + URL: "guber/bar", + PrevReportStates: map[string]v1.ProwJobState{ + "gerrit-reporter": v1.FailureState, + }, + }, + Spec: v1.ProwJobSpec{ + Refs: &v1.Refs{ + Repo: "bar", + }, + Job: "ci-bar", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + client.GerritRevision: "abc", + kube.ProwJobTypeLabel: "presubmit", + client.GerritReportLabel: "same-label", + }, + Annotations: map[string]string{ + client.GerritID: "123-abc", + client.GerritInstance: "gerrit", + }, + CreationTimestamp: metav1.Time{ + Time: timeNow.Add(-time.Hour), + }, + }, + Status: v1.ProwJobStatus{ + State: v1.FailureState, + URL: "guber/foo", + PrevReportStates: map[string]v1.ProwJobState{ + "gerrit-reporter": v1.FailureState, + }, + }, + Spec: v1.ProwJobSpec{ + Refs: &v1.Refs{ + Repo: "foo", + }, + Job: "ci-foo", + }, + }, + }, + expectReport: true, + reportInclude: []string{"1 out of 2", "ci-foo", "success", "ci-bar", "failure", "guber/foo", "guber/bar"}, + expectLabel: map[string]string{"same-label": client.LBTM}, + numExpectedReport: 2, }, } @@ -797,7 +911,7 @@ func TestReport(t *testing.T) { continue } - _, err := reporter.Report(tc.pj) + reportedJobs, err := reporter.Report(tc.pj) if err != nil { t.Errorf("test: %s: expect no error but got error %v", tc.name, err) } @@ -817,6 +931,9 @@ func TestReport(t *testing.T) { if !reflect.DeepEqual(tc.expectLabel, fgc.reportLabel) { t.Errorf("test: %s: reported with %s label, should have %s label", tc.name, fgc.reportLabel, tc.expectLabel) } + if len(reportedJobs) != tc.numExpectedReport { + t.Errorf("test: %s: reported with %d jobs, should have %d jobs instead", tc.name, len(reportedJobs), tc.numExpectedReport) + } } } } From b85461c9e30db342bb3438596a740d3d283092ec Mon Sep 17 00:00:00 2001 From: Amit Watve Date: Mon, 18 Mar 2019 14:42:36 -0700 Subject: [PATCH 08/10] Fix some comments and logging. --- prow/gerrit/adapter/trigger.go | 13 +++---------- prow/gerrit/reporter/reporter.go | 6 ++---- prow/pjutil/filter.go | 2 +- 3 files changed, 6 insertions(+), 15 deletions(-) diff --git a/prow/gerrit/adapter/trigger.go b/prow/gerrit/adapter/trigger.go index 4a9e0596e54e..7b59d0399c23 100644 --- a/prow/gerrit/adapter/trigger.go +++ b/prow/gerrit/adapter/trigger.go @@ -40,22 +40,15 @@ func messageFilter(lastUpdate time.Time, change client.ChangeInfo, presubmits [] } if message.RevisionNumber != currentRevision || messageTime.Before(lastUpdate) { continue - } - // Skip comments not germane to this plugin + if !pjutil.TestAllRe.MatchString(message.Message) { - matched := false for _, presubmit := range presubmits { - matched = matched || presubmit.TriggerMatches(message.Message) - if matched { + if presubmit.TriggerMatches(message.Message) { + logrus.Infof("Comment %s matches triggering regex, for %s.", message.Message, presubmit.Name) filters = append(filters, pjutil.CommandFilter(message.Message)) } } - if !matched { - logrus.Infof("Comment %s doesn't match any triggering regex, skipping.", message.Message) - continue - } - } else { filters = append(filters, pjutil.TestAllFilter()) } diff --git a/prow/gerrit/reporter/reporter.go b/prow/gerrit/reporter/reporter.go index bb57f97836f8..e104cc3da041 100644 --- a/prow/gerrit/reporter/reporter.go +++ b/prow/gerrit/reporter/reporter.go @@ -112,7 +112,6 @@ func (c *Client) ShouldReport(pj *v1.ProwJob) bool { // Report will send the current prowjob status as a gerrit review func (c *Client) Report(pj *v1.ProwJob) ([]*v1.ProwJob, error) { - // If you are hitting here, which means the entire patchset has been finished :-) logger := logrus.WithField("prowjob", pj) @@ -142,7 +141,6 @@ func (c *Client) Report(pj *v1.ProwJob) ([]*v1.ProwJob, error) { } // generate an aggregated report: - total := 0 success := 0 message := "" var toReportJobs []*v1.ProwJob @@ -158,14 +156,14 @@ func (c *Client) Report(pj *v1.ProwJob) ([]*v1.ProwJob, error) { continue } toReportJobs = append(toReportJobs, pjOnRevisionWithSameLabel) - total++ + if pjOnRevisionWithSameLabel.Status.State == v1.SuccessState { success++ } message = fmt.Sprintf("%s\nJob %s finished with %s -- URL: %s", message, pjOnRevisionWithSameLabel.Spec.Job, pjOnRevisionWithSameLabel.Status.State, pjOnRevisionWithSameLabel.Status.URL) } - + total := len(toReportJobs) if total <= 0 { // Shouldn't happen but return if does logger.Warn("Tried to report empty or aborted jobs.") diff --git a/prow/pjutil/filter.go b/prow/pjutil/filter.go index deb07b77139b..776951db2383 100644 --- a/prow/pjutil/filter.go +++ b/prow/pjutil/filter.go @@ -26,7 +26,7 @@ import ( var TestAllRe = regexp.MustCompile(`(?m)^/test all,?($|\s.*)`) // Filter digests a presubmit config to determine if: -// - we can be certain that the presubmit should run +// - we the presubmit matched the filter // - we know that the presubmit is forced to run // - what the default behavior should be if the presubmit // runs conditionally and does not match trigger conditions From ba6296093c082f03151f4f41b422a92c63f32b35 Mon Sep 17 00:00:00 2001 From: Amit Watve Date: Wed, 20 Mar 2019 14:26:44 -0700 Subject: [PATCH 09/10] Remove old revision filter. --- prow/gerrit/adapter/adapter.go | 9 ++++++++- prow/gerrit/adapter/trigger.go | 15 --------------- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/prow/gerrit/adapter/adapter.go b/prow/gerrit/adapter/adapter.go index 298439b9d9d0..1d9fd1da09f5 100644 --- a/prow/gerrit/adapter/adapter.go +++ b/prow/gerrit/adapter/adapter.go @@ -285,7 +285,14 @@ func (c *Controller) ProcessChange(instance string, change client.ChangeInfo) er } else { filters = append(filters, filter) } - filters = append(filters, oldRevisionFilter(c.lastUpdate, change.Revisions[change.CurrentRevision])) + latestRev := change.Revisions[change.CurrentRevision] + created, err := time.Parse(layout, latestRev.Created) + if err != nil { + logrus.WithError(err).Errorf("Parse time %v failed", latestRev.Created) + } + if created.After(c.lastUpdate) { + filters = append(filters, pjutil.TestAllFilter()) + } toTrigger, _, err := pjutil.FilterPresubmits(pjutil.AggregateFilter(filters), listChangedFiles(change), change.Branch, presubmits, logger) if err != nil { return fmt.Errorf("failed to filter presubmits: %v", err) diff --git a/prow/gerrit/adapter/trigger.go b/prow/gerrit/adapter/trigger.go index 7b59d0399c23..9dfa661cd90c 100644 --- a/prow/gerrit/adapter/trigger.go +++ b/prow/gerrit/adapter/trigger.go @@ -56,18 +56,3 @@ func messageFilter(lastUpdate time.Time, change client.ChangeInfo, presubmits [] } return pjutil.AggregateFilter(filters), nil } - -// oldRevisionFilter builds a filter for jobs that have already been processed for a revision. -func oldRevisionFilter(lastUpdate time.Time, rev client.RevisionInfo) pjutil.Filter { - created, err := time.Parse(layout, rev.Created) - if err != nil { - logrus.WithError(err).Errorf("Parse time %v failed", rev.Created) - return func(p config.Presubmit) (bool, bool, bool) { - return false, false, false - } - } - - return func(p config.Presubmit) (bool, bool, bool) { - return created.After(lastUpdate), false, false - } -} From c406b50a2a261bddfa48fd7f30b7dca382815b9d Mon Sep 17 00:00:00 2001 From: Amit Watve Date: Wed, 20 Mar 2019 14:45:09 -0700 Subject: [PATCH 10/10] Fix govet failures. --- prow/pubsub/subscriber/subscriber.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prow/pubsub/subscriber/subscriber.go b/prow/pubsub/subscriber/subscriber.go index e91aa6796b7c..45127d2f470b 100644 --- a/prow/pubsub/subscriber/subscriber.go +++ b/prow/pubsub/subscriber/subscriber.go @@ -162,7 +162,7 @@ func (s *Subscriber) handlePeriodicJob(l *logrus.Entry, msg messageInterface, su pj.Status.State = prowapi.ErrorState pj.Status.Description = err.Error() if s.Reporter.ShouldReport(&prowJob) { - if err := s.Reporter.Report(&prowJob); err != nil { + if _, err := s.Reporter.Report(&prowJob); err != nil { l.Warningf("failed to report status. %v", err) } }