From 63d1e7c0554d0bb7d667adbad93dd0d6ded8c4c4 Mon Sep 17 00:00:00 2001 From: Sebastien Vas Date: Wed, 18 Apr 2018 16:43:28 -0700 Subject: [PATCH] Use branch protection setting in tide --- prow/cmd/tide/README.md | 81 +++++++++++++++++ prow/config/branch_protection.go | 5 + prow/config/jobs.go | 10 ++ prow/config/tide.go | 4 + prow/tide/BUILD.bazel | 10 +- prow/tide/contextregister.go | 102 +++++++++++++++++++++ prow/tide/contextregister_test.go | 130 ++++++++++++++++++++++++++ prow/tide/tide.go | 84 +++++++++++------ prow/tide/tide_test.go | 146 ++++++++++++++++++++++++++++-- 9 files changed, 532 insertions(+), 40 deletions(-) create mode 100644 prow/cmd/tide/README.md create mode 100644 prow/tide/contextregister.go create mode 100644 prow/tide/contextregister_test.go diff --git a/prow/cmd/tide/README.md b/prow/cmd/tide/README.md new file mode 100644 index 0000000000000..cd06e801a85e8 --- /dev/null +++ b/prow/cmd/tide/README.md @@ -0,0 +1,81 @@ +# Tide Documentation + +Tide merges PR that match a given sets of criteria + +## Tide configuration + +Extend the primary prow [`config.yaml`] document to include a top-level +`tide` key that looks like the following: + +```yaml + +tide: + queries: + ... + branchProtection: + ... + merge_method: + ... + + +presubmits: + kubernetes/test-infra: + - name: fancy-job-name + context: fancy-job-name + always_run: true + spec: # podspec that runs job +``` + + +### Merging Options + +Tide supports all 3 github merging options: + +* squash +* merge +* rebase + +A merge method can be set for repo or per org. + +Example: + +```yaml +tide: + ... + merge_method: + org1: squash + org2/repo1: rebase + org2/repo2: merge +``` + +### Queries Configuration + +Queries are using github queries to find PRs to be merged. Multiple queries can be defined for a single repo. Queries support filtering per existing and missing labels. In order to filter PRs that have been approved, use the reviewApprovedRequired. + +```yaml +tide: + queries: + ... + - repos: + - org1/repo1 + - org2/repo2 + labels: + - labelThatMustsExists + - OtherLabelThatMustsExist + missingLabels: + - labelThatShouldNotBePresent + # If you want github approval + reviewApprovedRequired: true +``` + +### Branch Protection Options +Branch Protection options are use to enforce github branch protection. +A PR will be merged when all required checks are passing, meaning we will skip optional contexts. + +Example: + +```yaml +tide: + ... + skip_optional_contexts: true +``` diff --git a/prow/config/branch_protection.go b/prow/config/branch_protection.go index e6bab693d74d4..53cdfe4406589 100644 --- a/prow/config/branch_protection.go +++ b/prow/config/branch_protection.go @@ -43,3 +43,8 @@ type Branch struct { Contexts []string `json:"require-contexts,omitempty"` Pushers []string `json:"allow-push,omitempty"` } + +func (c *Config) GetBranchProtection(org, repo, branch string) (*Branch, error) { + // Place holder. Implemented in #7680 + return nil, nil +} diff --git a/prow/config/jobs.go b/prow/config/jobs.go index f9db0cb0c4d48..c111ff9f2dbfd 100644 --- a/prow/config/jobs.go +++ b/prow/config/jobs.go @@ -101,6 +101,8 @@ type Presubmit struct { Spec *v1.PodSpec `json:"spec,omitempty"` // Run these jobs after successfully running this one. RunAfterSuccess []Presubmit `json:"run_after_success"` + // Defines if a test is a required check as defined in the Github Branch protection. + Optional bool `json:"optional,omitempty"` Brancher @@ -207,6 +209,14 @@ func (ps Presubmit) TriggerMatches(body string) bool { return ps.re.MatchString(body) } +// ContextRequired checks whether a context is required from github points of view (required check). +func (ps Presubmit) ContextRequired() bool { + if ps.Optional || ps.SkipReport { + return false + } + return true +} + type ChangedFilesProvider func() ([]string, error) func matching(j Presubmit, body string, testAll bool) []Presubmit { diff --git a/prow/config/tide.go b/prow/config/tide.go index 49d789bbe4f5e..3021350a4df09 100644 --- a/prow/config/tide.go +++ b/prow/config/tide.go @@ -64,6 +64,10 @@ type Tide struct { // controller to handle org/repo:branch pools. Defaults to 20. Needs to be a // positive number. MaxGoroutines int `json:"max_goroutines,omitempty"` + + // SkipOptionalContexts will use BranchProtection configuration options to know + // which contexts can be skipped in order to merge a PR. + SkipOptionalContexts bool `json:"skip_optional_contexts,omitempty"` } // MergeMethod returns the merge method to use for a repo. The default of merge is diff --git a/prow/tide/BUILD.bazel b/prow/tide/BUILD.bazel index 6380fd3fa3aac..b8adec6e8f609 100644 --- a/prow/tide/BUILD.bazel +++ b/prow/tide/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 = ["tide.go"], + srcs = [ + "contextregister.go", + "tide.go", + ], importpath = "k8s.io/test-infra/prow/tide", visibility = ["//visibility:public"], deps = [ @@ -33,7 +36,10 @@ filegroup( go_test( name = "go_default_test", - srcs = ["tide_test.go"], + srcs = [ + "contextregister_test.go", + "tide_test.go", + ], embed = [":go_default_library"], deps = [ "//prow/config:go_default_library", diff --git a/prow/tide/contextregister.go b/prow/tide/contextregister.go new file mode 100644 index 0000000000000..0cee34de8b430 --- /dev/null +++ b/prow/tide/contextregister.go @@ -0,0 +1,102 @@ +/* +Copyright 2017 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 tide + +import ( + "sync" + + "github.com/shurcooL/githubql" + "k8s.io/apimachinery/pkg/util/sets" +) + +type ContextChecker interface { + // IgnoreContext tells whether a context is optional. + IgnoreContext(context Context) bool + // MissingRequiredContexts tells if required contexts are missing from the list of contexts provided. + MissingRequiredContexts([]Context) []Context +} + +// newExpectedContext creates a Context with Expected state. +// This should not only be used when contexts are missing. +func newExpectedContext(c string) Context { + return Context{ + Context: githubql.String(c), + State: githubql.StatusStateExpected, + Description: githubql.String(""), + } +} + +// ContextRegister implements ContextChecker and allow registering of required and optional contexts. +type ContextRegister struct { + lock sync.RWMutex + required, optional sets.String +} + +// NewContextRegister instantiates a new ContextRegister and register the optional contexts provided. +func NewContextRegister(optional ...string) *ContextRegister { + r := ContextRegister{ + required: sets.NewString(), + optional: sets.NewString(), + } + r.RegisterOptionalContexts(optional...) + return &r +} + +// IgnoreContext checks whether a context can be ignored. +// Will return true if +// - context is registered as optional +// - required contexts are registered and the context provided is not required +// Will return false otherwise. Every context is required. +func (r *ContextRegister) IgnoreContext(c Context) bool { + if r.optional.Has(string(c.Context)) { + return true + } + if r.required.Len() > 0 && !r.required.Has(string(c.Context)) { + return true + } + return false +} + +// MissingRequiredContexts discard the optional contexts and only look of extra required contexts that are not provided. +func (r *ContextRegister) MissingRequiredContexts(contexts []Context) []Context { + if r.required.Len() == 0 { + return nil + } + existingContexts := sets.NewString() + for _, c := range contexts { + existingContexts.Insert(string(c.Context)) + } + var missingContexts []Context + for c := range r.required.Difference(existingContexts) { + missingContexts = append(missingContexts, newExpectedContext(c)) + } + return missingContexts +} + +// RegisterOptionalContexts registers optional contexts +func (r *ContextRegister) RegisterOptionalContexts(c ...string) { + r.lock.Lock() + defer r.lock.Unlock() + r.optional.Insert(c...) +} + +// RegisterRequiredContexts register required contexts. +// Once required contexts are registered other contexts will be considered optional. +func (r *ContextRegister) RegisterRequiredContexts(c ...string) { + r.lock.Lock() + defer r.lock.Unlock() + r.required.Insert(c...) +} diff --git a/prow/tide/contextregister_test.go b/prow/tide/contextregister_test.go new file mode 100644 index 0000000000000..3058498c6a838 --- /dev/null +++ b/prow/tide/contextregister_test.go @@ -0,0 +1,130 @@ +/* +Copyright 2017 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 tide + +import ( + "testing" + + "k8s.io/apimachinery/pkg/util/sets" +) + +func TestContextRegisterIgnoreContext(t *testing.T) { + testCases := []struct { + name string + required, optional []string + contexts []string + results []bool + }{ + { + name: "only optional contexts registered", + contexts: []string{"c1", "o1", "o2"}, + optional: []string{"o1", "o2"}, + results: []bool{false, true, true}, + }, + { + name: "no contexts registered", + contexts: []string{"t2"}, + results: []bool{false}, + }, + { + name: "only required contexts registered", + required: []string{"c1", "c2", "c3"}, + contexts: []string{"c1", "c2", "c3", "t1"}, + results: []bool{false, false, false, true}, + }, + { + name: "optional and required contexts registered", + optional: []string{"o1", "o2"}, + required: []string{"c1", "c2", "c3"}, + contexts: []string{"o1", "o2", "c1", "c2", "c3", "t1"}, + results: []bool{true, true, false, false, false, true}, + }, + } + + for _, tc := range testCases { + cr := NewContextRegister(tc.optional...) + cr.RegisterRequiredContexts(tc.required...) + for i, c := range tc.contexts { + if cr.IgnoreContext(newExpectedContext(c)) != tc.results[i] { + t.Errorf("%s - ignoreContext for %s should return %t", tc.name, c, tc.results[i]) + } + } + } +} + +func contextsToSet(contexts []Context) sets.String { + s := sets.NewString() + for _, c := range contexts { + s.Insert(string(c.Context)) + } + return s +} + +func TestContextRegisterMissingContexts(t *testing.T) { + testCases := []struct { + name string + required, optional []string + existingContexts, expectedContexts []string + }{ + { + name: "no contexts registered", + existingContexts: []string{"c1", "c2"}, + }, + { + name: "optional contexts registered / no missing contexts", + optional: []string{"o1", "o2", "o3"}, + existingContexts: []string{"c1", "c2"}, + }, + { + name: "required contexts registered / missing contexts", + required: []string{"c1", "c2", "c3"}, + existingContexts: []string{"c1", "c2"}, + expectedContexts: []string{"c3"}, + }, + { + name: "required contexts registered / no missing contexts", + required: []string{"c1", "c2", "c3"}, + existingContexts: []string{"c1", "c2", "c3"}, + }, + { + name: "optional and required contexts registered / missing contexts", + optional: []string{"o1", "o2", "o3"}, + required: []string{"c1", "c2", "c3"}, + existingContexts: []string{"c1", "c2"}, + expectedContexts: []string{"c3"}, + }, + { + name: "optional and required contexts registered / no missing contexts", + optional: []string{"o1", "o2", "o3"}, + required: []string{"c1", "c2"}, + existingContexts: []string{"c1", "c2", "c4"}, + }, + } + + for _, tc := range testCases { + cr := NewContextRegister(tc.optional...) + cr.RegisterRequiredContexts(tc.required...) + var contexts []Context + for _, c := range tc.existingContexts { + contexts = append(contexts, newExpectedContext(c)) + } + missingContexts := cr.MissingRequiredContexts(contexts) + m := contextsToSet(missingContexts) + if !m.Equal(sets.NewString(tc.expectedContexts...)) { + t.Errorf("%s - expected %v got %v", tc.name, tc.expectedContexts, missingContexts) + } + } +} diff --git a/prow/tide/tide.go b/prow/tide/tide.go index c99ef63694b5b..cacb9f1483942 100644 --- a/prow/tide/tide.go +++ b/prow/tide/tide.go @@ -139,6 +139,14 @@ type Pool struct { Target []PullRequest } +// findRequiredContexts from a Branch protection +func findRequiredContexts(bp *config.Branch) []string { + if bp == nil || bp.Protect == nil || !*bp.Protect { + return nil + } + return bp.Contexts +} + // NewController makes a Controller out of the given clients. func NewController(ghcSync, ghcStatus *github.Client, kc *kube.Client, ca *config.Agent, gc *git.Client, logger *logrus.Entry) *Controller { if logger == nil { @@ -200,7 +208,7 @@ func byRepoAndNumber(prs []PullRequest) map[string]PullRequest { // Note: an empty diff can be returned if the reason that the PR does not match // the TideQuery is unknown. This can happen happen if this function's logic // does not match GitHub's and does not indicate that the PR matches the query. -func requirementDiff(pr *PullRequest, q *config.TideQuery) (string, int) { +func requirementDiff(pr *PullRequest, q *config.TideQuery, cc ContextChecker) (string, int) { const maxLabelChars = 50 var desc string var diff int @@ -285,7 +293,7 @@ func requirementDiff(pr *PullRequest, q *config.TideQuery) (string, int) { var contexts []string for _, commit := range pr.Commits.Nodes { if commit.Commit.OID == pr.HeadRefOID { - for _, ctx := range unsuccessfulContexts(commit.Commit.Status.Contexts) { + for _, ctx := range unsuccessfulContexts(commit.Commit.Status.Contexts, cc) { contexts = append(contexts, string(ctx.Context)) } } @@ -312,12 +320,12 @@ func requirementDiff(pr *PullRequest, q *config.TideQuery) (string, int) { // in order to generate a diff for the status description. We choose the query // for the repo that the PR is closest to meeting (as determined by the number // of unmet/violated requirements). -func expectedStatus(queriesByRepo map[string]config.TideQueries, pr *PullRequest, pool map[string]PullRequest) (string, string) { +func expectedStatus(queriesByRepo map[string]config.TideQueries, pr *PullRequest, pool map[string]PullRequest, cc ContextChecker) (string, string) { if _, ok := pool[prKey(pr)]; !ok { minDiffCount := -1 var minDiff string for _, q := range queriesByRepo[string(pr.Repository.NameWithOwner)] { - diff, diffCount := requirementDiff(pr, &q) + diff, diffCount := requirementDiff(pr, &q, cc) if minDiffCount == -1 || diffCount < minDiffCount { minDiffCount = diffCount minDiff = diff @@ -357,18 +365,28 @@ func (sc *statusController) setStatuses(all, pool []PullRequest) { process := func(pr *PullRequest) { processed.Insert(prKey(pr)) - log := sc.logger.WithFields(pr.logFields()) - contexts, err := headContexts(log, sc.ghc, pr) + hContexts, err := headContexts(log, sc.ghc, pr) if err != nil { log.WithError(err).Error("Getting head commit status contexts, skipping...") return } - - wantState, wantDesc := expectedStatus(queriesByRepo, pr, poolM) + cr := NewContextRegister(statusContext) + if sc.ca.Config().Tide.SkipOptionalContexts { + bp, err := sc.ca.Config().GetBranchProtection( + string(pr.Repository.Owner.Login), + string(pr.Repository.Name), + string(pr.BaseRef.Name)) + if err != nil { + log.WithError(err).Error("Getting branch protection") + return + } + cr.RegisterRequiredContexts(findRequiredContexts(bp)...) + } + wantState, wantDesc := expectedStatus(queriesByRepo, pr, poolM, cr) var actualState githubql.StatusState var actualDesc string - for _, ctx := range contexts { + for _, ctx := range hContexts { if string(ctx.Context) == statusContext { actualState = ctx.State actualDesc = string(ctx.Description) @@ -569,7 +587,7 @@ func toSimpleState(s kube.ProwJobState) simpleState { // isPassingTests returns whether or not all contexts set on the PR except for // the tide pool context are passing. -func isPassingTests(log *logrus.Entry, ghc githubClient, pr PullRequest) bool { +func isPassingTests(log *logrus.Entry, ghc githubClient, pr PullRequest, cc ContextChecker) bool { log = log.WithFields(pr.logFields()) contexts, err := headContexts(log, ghc, &pr) if err != nil { @@ -577,26 +595,29 @@ func isPassingTests(log *logrus.Entry, ghc githubClient, pr PullRequest) bool { // If we can't get the status of the commit, assume that it is failing. return false } - return len(unsuccessfulContexts(contexts)) == 0 + return len(unsuccessfulContexts(contexts, cc)) == 0 } -// unsuccessfulContexts determines which contexts from the list are failed that -// we care about. For instance, we do not care about our own context. -func unsuccessfulContexts(contexts []Context) []Context { +// unsuccessfulContexts determines which contexts from the list that we care about are +// failed. For instance, we do not care about our own context. +// If the branchProtection is set to only check for required checks, we will skip +// all non-required tests. If required tests are missing from the list, they will be +// added to the list of failed contexts. +func unsuccessfulContexts(contexts []Context, cc ContextChecker) []Context { var failed []Context for _, ctx := range contexts { - if string(ctx.Context) == statusContext { + if cc.IgnoreContext(ctx) { continue } if ctx.State != githubql.StatusStateSuccess { failed = append(failed, ctx) } } - + failed = append(failed, cc.MissingRequiredContexts(contexts)...) return failed } -func pickSmallestPassingNumber(log *logrus.Entry, ghc githubClient, prs []PullRequest) (bool, PullRequest) { +func pickSmallestPassingNumber(log *logrus.Entry, ghc githubClient, prs []PullRequest, cc ContextChecker) (bool, PullRequest) { smallestNumber := -1 var smallestPR PullRequest for _, pr := range prs { @@ -606,7 +627,7 @@ func pickSmallestPassingNumber(log *logrus.Entry, ghc githubClient, prs []PullRe if len(pr.Commits.Nodes) < 1 { continue } - if !isPassingTests(log, ghc, pr) { + if !isPassingTests(log, ghc, pr, cc) { continue } smallestNumber = int(pr.Number) @@ -746,7 +767,7 @@ func prNumbers(prs []PullRequest) []int { return nums } -func (c *Controller) pickBatch(sp subpool) ([]PullRequest, error) { +func (c *Controller) pickBatch(sp subpool, cc ContextChecker) ([]PullRequest, error) { r, err := c.gc.Clone(sp.org + "/" + sp.repo) if err != nil { return nil, err @@ -770,7 +791,7 @@ func (c *Controller) pickBatch(sp subpool) ([]PullRequest, error) { var res []PullRequest for _, pr := range sp.prs { - if !isPassingTests(sp.log, c.ghc, pr) { + if !isPassingTests(sp.log, c.ghc, pr, cc) { continue } if ok, err := r.Merge(string(pr.HeadRefOID)); err != nil { @@ -889,7 +910,7 @@ func (c *Controller) trigger(sp subpool, presubmits map[int]sets.String, prs []P return nil } -func (c *Controller) takeAction(sp subpool, presubmits map[int]sets.String, batchPending, successes, pendings, nones, batchMerges []PullRequest) (Action, []PullRequest, error) { +func (c *Controller) takeAction(sp subpool, presubmits map[int]sets.String, batchPending, successes, pendings, nones, batchMerges []PullRequest, cc ContextChecker) (Action, []PullRequest, error) { // Merge the batch! if len(batchMerges) > 0 { return MergeBatch, batchMerges, c.mergePRs(sp, batchMerges) @@ -897,19 +918,19 @@ func (c *Controller) takeAction(sp subpool, presubmits map[int]sets.String, batc // Do not merge PRs while waiting for a batch to complete. We don't want to // invalidate the old batch result. if len(successes) > 0 && len(batchPending) == 0 { - if ok, pr := pickSmallestPassingNumber(sp.log, c.ghc, successes); ok { + if ok, pr := pickSmallestPassingNumber(sp.log, c.ghc, successes, cc); ok { return Merge, []PullRequest{pr}, c.mergePRs(sp, []PullRequest{pr}) } } // If we have no serial jobs pending or successful, trigger one. if len(nones) > 0 && len(pendings) == 0 && len(successes) == 0 { - if ok, pr := pickSmallestPassingNumber(sp.log, c.ghc, nones); ok { + if ok, pr := pickSmallestPassingNumber(sp.log, c.ghc, nones, cc); ok { return Trigger, []PullRequest{pr}, c.trigger(sp, presubmits, []PullRequest{pr}) } } // If we have no batch, trigger one. if len(sp.prs) > 1 && len(batchPending) == 0 { - batch, err := c.pickBatch(sp) + batch, err := c.pickBatch(sp, cc) if err != nil { return Wait, nil, err } @@ -936,7 +957,7 @@ func (c *Controller) presubmitsByPull(sp subpool) (map[int]sets.String, error) { }() for _, ps := range c.ca.Config().Presubmits[sp.org+"/"+sp.repo] { - if ps.SkipReport || !ps.RunsAgainstBranch(sp.branch) { + if !ps.ContextRequired() || !ps.RunsAgainstBranch(sp.branch) { continue } @@ -972,11 +993,18 @@ func (c *Controller) presubmitsByPull(sp subpool) (map[int]sets.String, error) { } func (c *Controller) syncSubpool(sp subpool) (Pool, error) { - sp.log.Infof("Syncing subpool: %d PRs, %d PJs.", len(sp.prs), len(sp.pjs)) presubmits, err := c.presubmitsByPull(sp) if err != nil { return Pool{}, fmt.Errorf("error determining required presubmits: %v", err) } + cr := NewContextRegister(statusContext) + if c.ca.Config().Tide.SkipOptionalContexts { + bp, err := c.ca.Config().GetBranchProtection(sp.org, sp.repo, sp.branch) + if err != nil { + return Pool{}, fmt.Errorf("error parsing branch protection: %v", err) + } + cr.RegisterRequiredContexts(findRequiredContexts(bp)...) + } successes, pendings, nones := accumulate(presubmits, sp.prs, sp.pjs) batchMerge, batchPending := accumulateBatch(presubmits, sp.prs, sp.pjs) sp.log.WithFields(logrus.Fields{ @@ -986,8 +1014,7 @@ func (c *Controller) syncSubpool(sp subpool) (Pool, error) { "batch-passing": prNumbers(batchMerge), "batch-pending": prNumbers(batchPending), }).Info("Subpool accumulated.") - - act, targets, err := c.takeAction(sp, presubmits, batchPending, successes, pendings, nones, batchMerge) + act, targets, err := c.takeAction(sp, presubmits, batchPending, successes, pendings, nones, batchMerge, cr) sp.log.WithFields(logrus.Fields{ "action": string(act), "targets": prNumbers(targets), @@ -1195,7 +1222,6 @@ func headContexts(log *logrus.Entry, ghc githubClient, pr *PullRequest) ([]Conte if err != nil { return nil, fmt.Errorf("failed to get the combined status: %v", err) } - contexts := make([]Context, 0, len(combined.Statuses)) for _, status := range combined.Statuses { contexts = append( diff --git a/prow/tide/tide_test.go b/prow/tide/tide_test.go index 2134397a40671..b32ae2eac1b0e 100644 --- a/prow/tide/tide_test.go +++ b/prow/tide/tide_test.go @@ -379,7 +379,8 @@ type fgc struct { merged int setStatus bool - expectedSHA string + expectedSHA string + combinedStatus map[string]string } func (f *fgc) GetRef(o, r, ref string) (string, error) { @@ -423,10 +424,12 @@ func (f *fgc) GetCombinedStatus(org, repo, ref string) (*github.CombinedStatus, if f.expectedSHA != ref { return nil, errors.New("bad combined status request: incorrect sha") } + var statuses []github.Status + for c, s := range f.combinedStatus { + statuses = append(statuses, github.Status{Context: c, State: s}) + } return &github.CombinedStatus{ - Statuses: []github.Status{ - {Context: "win"}, - }, + Statuses: statuses, }, nil } @@ -621,7 +624,7 @@ func TestExpectedStatus(t *testing.T) { pool = map[string]PullRequest{"#0": {}} } - state, desc := expectedStatus(queriesByRepo, &pr, pool) + state, desc := expectedStatus(queriesByRepo, &pr, pool, NewContextRegister(statusContext)) if state != tc.state { t.Errorf("Expected status state %q, but got %q.", string(tc.state), string(state)) } @@ -1000,11 +1003,14 @@ func TestPickBatch(t *testing.T) { } sp.prs = append(sp.prs, pr) } + ca := &config.Agent{} + ca.Set(&config.Config{}) c := &Controller{ logger: logrus.WithField("component", "tide"), gc: gc, + ca: ca, } - prs, err := c.pickBatch(sp) + prs, err := c.pickBatch(sp, NewContextRegister(statusContext)) if err != nil { t.Fatalf("Error from pickBatch: %v", err) } @@ -1255,7 +1261,8 @@ func TestTakeAction(t *testing.T) { batchPending = []PullRequest{{}} } t.Logf("Test case: %s", tc.name) - if act, _, err := c.takeAction(sp, presubmits, batchPending, genPulls(tc.successes), genPulls(tc.pendings), genPulls(tc.nones), genPulls(tc.batchMerges)); err != nil { + cr := NewContextRegister(statusContext) + if act, _, err := c.takeAction(sp, presubmits, batchPending, genPulls(tc.successes), genPulls(tc.pendings), genPulls(tc.nones), genPulls(tc.batchMerges), cr); err != nil { t.Errorf("Error in takeAction: %v", err) continue } else if act != tc.action { @@ -1358,7 +1365,7 @@ func TestHeadContexts(t *testing.T) { for _, tc := range testCases { t.Logf("Running test case %q", tc.name) - fgc := &fgc{} + fgc := &fgc{combinedStatus: map[string]string{win: string(githubql.StatusStateSuccess)}} if tc.expectAPICall { fgc.expectedSHA = headSHA } @@ -1532,6 +1539,76 @@ func TestSync(t *testing.T) { } } +func TestIsPassing(t *testing.T) { + yes := true + no := false + headSHA := "head" + success := string(githubql.StatusStateSuccess) + failure := string(githubql.StatusStateFailure) + testCases := []struct { + name string + passing bool + config *config.Branch + combinedContexts map[string]string + }{ + { + name: "required checks disabled should not impact - passing", + passing: true, + config: &config.Branch{ + Protect: &no, + Contexts: []string{"c1", "c2", "c3"}, + }, + combinedContexts: map[string]string{"c1": success, "c2": success, statusContext: failure}, + }, + { + name: "required checks disabled should not impact - failing", + passing: false, + combinedContexts: map[string]string{"c1": success, "c2": failure}, + config: &config.Branch{ + Protect: &no, + Contexts: []string{"c1", "c2", "c3"}, + }, + }, + { + name: "required checks used - passing", + passing: true, + combinedContexts: map[string]string{"c1": success, "c2": failure, "c3": success}, + config: &config.Branch{ + Protect: &yes, + Contexts: []string{"c1", "c3"}, + }, + }, + { + name: "required checks used - failing", + passing: false, + combinedContexts: map[string]string{"c1": success, "c2": success}, + config: &config.Branch{ + Protect: &yes, + Contexts: []string{"c1", "c2", "c3"}, + }, + }, + } + + for _, tc := range testCases { + ghc := &fgc{ + combinedStatus: tc.combinedContexts, + expectedSHA: headSHA} + log := logrus.WithField("component", "tide") + _, err := log.String() + if err != nil { + t.Errorf("Failed to get log output before testing: %v", err) + t.FailNow() + } + cr := NewContextRegister(statusContext) + cr.RegisterRequiredContexts(findRequiredContexts(tc.config)...) + pr := PullRequest{HeadRefOID: githubql.String(headSHA)} + passing := isPassingTests(log, ghc, pr, cr) + if passing != tc.passing { + t.Errorf("%s: Expected %t got %t", tc.name, tc.passing, passing) + } + } +} + func TestTargetUrl(t *testing.T) { testcases := []struct { name string @@ -1781,7 +1858,6 @@ func TestPresubmitsByPull(t *testing.T) { fileChangesCache: tc.initialChangeCache, ghc: &fgc{}, } - presubmits, err := c.presubmitsByPull(sp) if err != nil { t.Fatalf("unexpected error from presubmitsByPull: %v", err) @@ -1794,3 +1870,55 @@ func TestPresubmitsByPull(t *testing.T) { } } } + +func TestFindRequiredContexts(t *testing.T) { + yes := true + no := false + testCases := []struct { + name string + config *config.Branch + results []string + }{ + { + name: "no config", + }, + { + name: "config protect false missing context", + config: &config.Branch{Protect: &no}, + }, + { + name: "config existing context", + config: &config.Branch{ + Protect: &yes, + Contexts: []string{"c1", "c2", "c3"}, + }, + results: []string{"c1", "c2", "c3"}, + }, + { + name: "config existing context protect disabled", + config: &config.Branch{ + Protect: &no, + Contexts: []string{"c1", "c2", "c3"}, + }, + }, + { + name: "config existing context nil protection", + config: &config.Branch{ + Contexts: []string{"c1", "c2", "c3"}, + }, + }, + { + name: "config missing context protect enabled", + config: &config.Branch{ + Protect: &yes, + }, + }, + } + + for _, tc := range testCases { + r := findRequiredContexts(tc.config) + if !reflect.DeepEqual(r, tc.results) { + t.Errorf("%s - expected contexts %v got %v", tc.name, tc.results, r) + } + } +}