From 213e69424fe96c007006b5f4395f6a51b92dcebc Mon Sep 17 00:00:00 2001 From: Derek Jobst Date: Thu, 14 Jan 2021 15:15:36 -0800 Subject: [PATCH] Add support for enforcing regex rules on PR Title (#256) This PR includes changes which add functionality for disapproving pull requests which do not comply by defined title formatting requirements as mentioned in https://github.com/palantir/policy-bot/issues/244. - A new predicate `title` is added for defining allowed (`not_match`) and disallowed (`match`) regex patterns on a pull request title. - The `disapproval` policy is extended to allow predicates just as individual `approval_rules` do. However, whereas an approval rule may only allow approvals subject to passing predicates, the `disapproval` policy will only allow disapprovals subject to its own predicates all failing. Passing predicates on the `disapproval` policy will trigger a default disapproval, just as failing predicates on an approval rule will implicitly approve (pass). --- README.md | 37 +++++- policy/approval/approve.go | 13 +- policy/approval/evaluate_test.go | 2 +- policy/approval/predicate.go | 83 ------------- policy/disapproval/disapprove.go | 48 ++++++-- policy/disapproval/disapprove_test.go | 50 ++++++-- policy/predicate/file.go | 11 +- policy/predicate/matches.go | 28 +++++ policy/predicate/predicates.go | 85 +++++++++++++ policy/predicate/title.go | 51 ++++++++ policy/predicate/title_test.go | 169 ++++++++++++++++++++++++++ pull/context.go | 3 + pull/github.go | 9 +- pull/github_test.go | 1 + pull/pulltest/context.go | 5 + 15 files changed, 471 insertions(+), 124 deletions(-) delete mode 100644 policy/approval/predicate.go create mode 100644 policy/predicate/matches.go create mode 100644 policy/predicate/predicates.go create mode 100644 policy/predicate/title.go create mode 100644 policy/predicate/title_test.go diff --git a/README.md b/README.md index f902b9d7..e7d3a742 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ UI to view the detailed approval status of any pull request. + [policy.yml Specification](#policyyml-specification) + [Approval Rules](#approval-rules) + [Approval Policies](#approval-policies) - + [Disapproval](#disapproval) + + [Disapproval Policy](#disapproval-policy) + [Caveats and Notes](#caveats-and-notes) - [Disapproval is Disabled by Default](#disapproval-is-disabled-by-default) - [`or`, `and`, and `if` (Rule Predicates)](#or-and-and-if-rule-predicates) @@ -199,6 +199,18 @@ if: has_labels: - "label-1" - "label-2" + + # "title" is satisfied if the pull request title matches any one of the + # patterns within the "matches" list, and does not match all of the patterns + # within the "not_matches" list. + # e.g. this predicate triggers for titles including "BREAKING CHANGE" or titles + # that are not marked as docs/style/chore changes (using conventional commits + # formatting) + title: + matches: + - "^BREAKING CHANGE: (\\w| )+$" + not_matches: + - "^(docs|style|chore): (\\w| )+$" # "options" specifies a set of restrictions on approvals. If the block does not # exist, the default values are used. @@ -325,19 +337,34 @@ Conjunctions can contain more conjunctions (up to a maximum depth of 5): - rule4 ``` -### Disapproval +### Disapproval Policy Disapproval allows users to explicitly block pull requests if certain changes must be made. Any member of in the set of allowed users can disapprove a change or revoke another user's disapproval. -Unlike approval, all disapproval options are specified as part of the policy. -Effectively, there is a single disapproval rule. The `disapproval` policy has -the following specification: +Unlike approval, all disapproval predicates and options are specified as part +of the policy. Effectively, there is a single disapproval rule. The `disapproval` +policy has the following specification: ```yaml # "disapproval" is the top-level key in the policy block. disapproval: + # "if" specifies a set of predicates which will cause disapproval if any are + # true + # + # This block, and every condition within it are optional. If the block does + # not exist, a pull request is only disapproved if a user takes a disapproval + # action. + if: + # All predicates from the approval rules section are valid here + title: + not_matches: + - "^(fix|feat|chore): (\\w| )+$" + - "^BREAKING CHANGE: (\\w| )+$" + matches: + - "^BLOCKED" + # "options" sets behavior related to disapproval. If it does not exist, the # defaults shown below are used. options: diff --git a/policy/approval/approve.go b/policy/approval/approve.go index 6c6eca0f..1d072d22 100644 --- a/policy/approval/approve.go +++ b/policy/approval/approve.go @@ -25,15 +25,16 @@ import ( "github.com/rs/zerolog" "github.com/palantir/policy-bot/policy/common" + "github.com/palantir/policy-bot/policy/predicate" "github.com/palantir/policy-bot/pull" ) type Rule struct { - Name string `yaml:"name"` - Description string `yaml:"description"` - Predicates Predicates `yaml:"if"` - Options Options `yaml:"options"` - Requires Requires `yaml:"requires"` + Name string `yaml:"name"` + Description string `yaml:"description"` + Predicates predicate.Predicates `yaml:"if"` + Options Options `yaml:"options"` + Requires Requires `yaml:"requires"` } type Options struct { @@ -115,7 +116,7 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res res.StatusDescription = desc if desc == "" { - res.StatusDescription = "The preconditions of this rule are not satisfied" + res.StatusDescription = "A precondition of this rule was satisfied" } return diff --git a/policy/approval/evaluate_test.go b/policy/approval/evaluate_test.go index 655ef7a0..e8fb1320 100644 --- a/policy/approval/evaluate_test.go +++ b/policy/approval/evaluate_test.go @@ -67,7 +67,7 @@ func TestRules(t *testing.T) { expected := []*Rule{ { Name: "rule1", - Predicates: Predicates{ + Predicates: predicate.Predicates{ ChangedFiles: &predicate.ChangedFiles{ Paths: []common.Regexp{ common.NewCompiledRegexp(regexp.MustCompile("path1")), diff --git a/policy/approval/predicate.go b/policy/approval/predicate.go deleted file mode 100644 index e31d9a2a..00000000 --- a/policy/approval/predicate.go +++ /dev/null @@ -1,83 +0,0 @@ -// Copyright 2018 Palantir Technologies, Inc. -// -// 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 approval - -import ( - "github.com/palantir/policy-bot/policy/predicate" -) - -type Predicates struct { - ChangedFiles *predicate.ChangedFiles `yaml:"changed_files"` - OnlyChangedFiles *predicate.OnlyChangedFiles `yaml:"only_changed_files"` - - HasAuthorIn *predicate.HasAuthorIn `yaml:"has_author_in"` - HasContributorIn *predicate.HasContributorIn `yaml:"has_contributor_in"` - OnlyHasContributorsIn *predicate.OnlyHasContributorsIn `yaml:"only_has_contributors_in"` - AuthorIsOnlyContributor *predicate.AuthorIsOnlyContributor `yaml:"author_is_only_contributor"` - - TargetsBranch *predicate.TargetsBranch `yaml:"targets_branch"` - FromBranch *predicate.FromBranch `yaml:"from_branch"` - - ModifiedLines *predicate.ModifiedLines `yaml:"modified_lines"` - - HasSuccessfulStatus *predicate.HasSuccessfulStatus `yaml:"has_successful_status"` - - HasLabels *predicate.HasLabels `yaml:"has_labels"` -} - -func (p *Predicates) Predicates() []predicate.Predicate { - var ps []predicate.Predicate - - if p.ChangedFiles != nil { - ps = append(ps, predicate.Predicate(p.ChangedFiles)) - } - if p.OnlyChangedFiles != nil { - ps = append(ps, predicate.Predicate(p.OnlyChangedFiles)) - } - - if p.HasAuthorIn != nil { - ps = append(ps, predicate.Predicate(p.HasAuthorIn)) - } - if p.HasContributorIn != nil { - ps = append(ps, predicate.Predicate(p.HasContributorIn)) - } - if p.OnlyHasContributorsIn != nil { - ps = append(ps, predicate.Predicate(p.OnlyHasContributorsIn)) - } - if p.AuthorIsOnlyContributor != nil { - ps = append(ps, predicate.Predicate(p.AuthorIsOnlyContributor)) - } - - if p.TargetsBranch != nil { - ps = append(ps, predicate.Predicate(p.TargetsBranch)) - } - if p.FromBranch != nil { - ps = append(ps, predicate.Predicate(p.FromBranch)) - } - - if p.ModifiedLines != nil { - ps = append(ps, predicate.Predicate(p.ModifiedLines)) - } - - if p.HasSuccessfulStatus != nil { - ps = append(ps, predicate.Predicate(p.HasSuccessfulStatus)) - } - - if p.HasLabels != nil { - ps = append(ps, predicate.Predicate(p.HasLabels)) - } - - return ps -} diff --git a/policy/disapproval/disapprove.go b/policy/disapproval/disapprove.go index 64f1ed12..2d54c094 100644 --- a/policy/disapproval/disapprove.go +++ b/policy/disapproval/disapprove.go @@ -23,12 +23,14 @@ import ( "github.com/rs/zerolog" "github.com/palantir/policy-bot/policy/common" + "github.com/palantir/policy-bot/policy/predicate" "github.com/palantir/policy-bot/pull" ) type Policy struct { - Options Options `yaml:"options"` - Requires Requires `yaml:"requires"` + Predicates predicate.Predicates `yaml:"if"` + Options Options `yaml:"options"` + Requires Requires `yaml:"requires"` } type Options struct { @@ -77,16 +79,24 @@ type Requires struct { } func (p *Policy) Trigger() common.Trigger { - dm := p.Options.GetDisapproveMethods() - rm := p.Options.GetRevokeMethods() - t := common.TriggerCommit - if len(dm.Comments) > 0 || len(rm.Comments) > 0 { - t |= common.TriggerComment + + if !p.Requires.IsEmpty() { + dm := p.Options.GetDisapproveMethods() + rm := p.Options.GetRevokeMethods() + + if len(dm.Comments) > 0 || len(rm.Comments) > 0 { + t |= common.TriggerComment + } + if dm.GithubReview || rm.GithubReview { + t |= common.TriggerReview + } } - if dm.GithubReview || rm.GithubReview { - t |= common.TriggerReview + + for _, predicate := range p.Predicates.Predicates() { + t |= predicate.Trigger() } + return t } @@ -96,6 +106,26 @@ func (p *Policy) Evaluate(ctx context.Context, prctx pull.Context) (res common.R res.Name = "disapproval" res.Status = common.StatusSkipped + for _, p := range p.Predicates.Predicates() { + satisfied, desc, err := p.Evaluate(ctx, prctx) + + if err != nil { + res.Error = errors.Wrap(err, "failed to evaluate predicate") + return + } + + if satisfied { + log.Debug().Msgf("disapproving, predicate of type %T was satisfied", p) + + res.Status = common.StatusDisapproved + res.StatusDescription = desc + if desc == "" { + res.StatusDescription = "A precondition of this rule was satisfied" + } + return + } + } + if p.Requires.IsEmpty() { log.Debug().Msg("no users are allowed to disapprove; skipping") diff --git a/policy/disapproval/disapprove_test.go b/policy/disapproval/disapprove_test.go index 7655cd3e..b276a4ae 100644 --- a/policy/disapproval/disapprove_test.go +++ b/policy/disapproval/disapprove_test.go @@ -17,6 +17,7 @@ package disapproval import ( "context" "os" + "regexp" "testing" "time" @@ -24,6 +25,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/palantir/policy-bot/policy/common" + "github.com/palantir/policy-bot/policy/predicate" "github.com/palantir/policy-bot/pull" "github.com/palantir/policy-bot/pull/pulltest" ) @@ -33,6 +36,7 @@ func TestIsDisapproved(t *testing.T) { ctx := logger.WithContext(context.Background()) prctx := &pulltest.Context{ + TitleValue: "test: add disapproval predicate test", CommentsValue: []*pull.Comment{ { Author: "disapprover-1", @@ -75,26 +79,28 @@ func TestIsDisapproved(t *testing.T) { } assertDisapproved := func(t *testing.T, p *Policy, expected string) { - disapproved, msg, err := p.IsDisapproved(ctx, prctx) - require.NoError(t, err) + res := p.Evaluate(ctx, prctx) - if assert.True(t, disapproved, "pull request was not disapproved") { - assert.Equal(t, expected, msg) + require.NoError(t, res.Error) + + if assert.Equal(t, common.StatusDisapproved, res.Status, "pull request was not disapproved") { + assert.Equal(t, expected, res.StatusDescription) } } assertSkipped := func(t *testing.T, p *Policy, expected string) { - disapproved, msg, err := p.IsDisapproved(ctx, prctx) - require.NoError(t, err) + res := p.Evaluate(ctx, prctx) + + require.NoError(t, res.Error) - if assert.False(t, disapproved, "pull request was incorrectly disapproved") { - assert.Equal(t, expected, msg) + if assert.Equal(t, common.StatusSkipped, res.Status, "pull request was incorrectly disapproved") { + assert.Equal(t, expected, res.StatusDescription) } } t.Run("skippedWithNoRequires", func(t *testing.T) { p := &Policy{} - assertSkipped(t, p, "No disapprovals") + assertSkipped(t, p, "No disapproval policy is specified or the policy is empty") }) t.Run("singleUserDisapproves", func(t *testing.T) { @@ -145,6 +151,32 @@ func TestIsDisapproved(t *testing.T) { assertDisapproved(t, p, "Disapproved by disapprover-4") }) + + t.Run("predicateDisapproves", func(t *testing.T) { + p := &Policy{} + p.Predicates = predicate.Predicates{ + Title: &predicate.Title{ + NotMatches: []common.Regexp{ + common.NewCompiledRegexp(regexp.MustCompile("^(fix|feat|docs)")), + }, + }, + } + + assertDisapproved(t, p, "Title doesn't match a NotMatch pattern") + }) + + t.Run("predicateDoesNotDisapprove", func(t *testing.T) { + p := &Policy{} + p.Predicates = predicate.Predicates{ + Title: &predicate.Title{ + NotMatches: []common.Regexp{ + common.NewCompiledRegexp(regexp.MustCompile("^(fix|feat|docs|test)")), + }, + }, + } + + assertSkipped(t, p, "No disapproval policy is specified or the policy is empty") + }) } func date(hour int) time.Time { diff --git a/policy/predicate/file.go b/policy/predicate/file.go index c31fdc19..02a039d6 100644 --- a/policy/predicate/file.go +++ b/policy/predicate/file.go @@ -45,7 +45,7 @@ func (pred *ChangedFiles) Evaluate(ctx context.Context, prctx pull.Context) (boo } if anyMatches(pred.Paths, f.Filename) { - return true, "", nil + return true, f.Filename + " was changed", nil } } @@ -163,12 +163,3 @@ func (pred *ModifiedLines) Trigger() common.Trigger { } var _ Predicate = &ModifiedLines{} - -func anyMatches(re []common.Regexp, s string) bool { - for _, r := range re { - if r.Matches(s) { - return true - } - } - return false -} diff --git a/policy/predicate/matches.go b/policy/predicate/matches.go new file mode 100644 index 00000000..a99025a5 --- /dev/null +++ b/policy/predicate/matches.go @@ -0,0 +1,28 @@ +// Copyright 2021 Palantir Technologies, Inc. +// +// 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 predicate + +import ( + "github.com/palantir/policy-bot/policy/common" +) + +func anyMatches(re []common.Regexp, s string) bool { + for _, r := range re { + if r.Matches(s) { + return true + } + } + return false +} diff --git a/policy/predicate/predicates.go b/policy/predicate/predicates.go new file mode 100644 index 00000000..7c24d6da --- /dev/null +++ b/policy/predicate/predicates.go @@ -0,0 +1,85 @@ +// Copyright 2021 Palantir Technologies, Inc. +// +// 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 predicate + +type Predicates struct { + ChangedFiles *ChangedFiles `yaml:"changed_files"` + OnlyChangedFiles *OnlyChangedFiles `yaml:"only_changed_files"` + + HasAuthorIn *HasAuthorIn `yaml:"has_author_in"` + HasContributorIn *HasContributorIn `yaml:"has_contributor_in"` + OnlyHasContributorsIn *OnlyHasContributorsIn `yaml:"only_has_contributors_in"` + AuthorIsOnlyContributor *AuthorIsOnlyContributor `yaml:"author_is_only_contributor"` + + TargetsBranch *TargetsBranch `yaml:"targets_branch"` + FromBranch *FromBranch `yaml:"from_branch"` + + ModifiedLines *ModifiedLines `yaml:"modified_lines"` + + HasSuccessfulStatus *HasSuccessfulStatus `yaml:"has_successful_status"` + + HasLabels *HasLabels `yaml:"has_labels"` + + Title *Title `yaml:"title"` +} + +func (p *Predicates) Predicates() []Predicate { + var ps []Predicate + + if p.ChangedFiles != nil { + ps = append(ps, Predicate(p.ChangedFiles)) + } + if p.OnlyChangedFiles != nil { + ps = append(ps, Predicate(p.OnlyChangedFiles)) + } + + if p.HasAuthorIn != nil { + ps = append(ps, Predicate(p.HasAuthorIn)) + } + if p.HasContributorIn != nil { + ps = append(ps, Predicate(p.HasContributorIn)) + } + if p.OnlyHasContributorsIn != nil { + ps = append(ps, Predicate(p.OnlyHasContributorsIn)) + } + if p.AuthorIsOnlyContributor != nil { + ps = append(ps, Predicate(p.AuthorIsOnlyContributor)) + } + + if p.TargetsBranch != nil { + ps = append(ps, Predicate(p.TargetsBranch)) + } + if p.FromBranch != nil { + ps = append(ps, Predicate(p.FromBranch)) + } + + if p.ModifiedLines != nil { + ps = append(ps, Predicate(p.ModifiedLines)) + } + + if p.HasSuccessfulStatus != nil { + ps = append(ps, Predicate(p.HasSuccessfulStatus)) + } + + if p.HasLabels != nil { + ps = append(ps, Predicate(p.HasLabels)) + } + + if p.Title != nil { + ps = append(ps, Predicate(p.Title)) + } + + return ps +} diff --git a/policy/predicate/title.go b/policy/predicate/title.go new file mode 100644 index 00000000..530b4b41 --- /dev/null +++ b/policy/predicate/title.go @@ -0,0 +1,51 @@ +// Copyright 2021 Palantir Technologies, Inc. +// +// 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 predicate + +import ( + "context" + + "github.com/palantir/policy-bot/policy/common" + "github.com/palantir/policy-bot/pull" +) + +type Title struct { + Matches []common.Regexp `yaml:"matches"` + NotMatches []common.Regexp `yaml:"not_matches"` +} + +var _ Predicate = Title{} + +func (pred Title) Evaluate(ctx context.Context, prctx pull.Context) (bool, string, error) { + title := prctx.Title() + + if len(pred.Matches) > 0 { + if anyMatches(pred.Matches, title) { + return true, "Title matches a Match pattern", nil + } + } + + if len(pred.NotMatches) > 0 { + if !anyMatches(pred.NotMatches, title) { + return true, "Title doesn't match a NotMatch pattern", nil + } + } + + return false, "", nil +} + +func (pred Title) Trigger() common.Trigger { + return common.TriggerPullRequest +} diff --git a/policy/predicate/title_test.go b/policy/predicate/title_test.go new file mode 100644 index 00000000..5126bb9a --- /dev/null +++ b/policy/predicate/title_test.go @@ -0,0 +1,169 @@ +// Copyright 2021 Palantir Technologies, Inc. +// +// 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 predicate + +import ( + "context" + "regexp" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/palantir/policy-bot/policy/common" + "github.com/palantir/policy-bot/pull" + "github.com/palantir/policy-bot/pull/pulltest" +) + +func TestWithNotMatchRule(t *testing.T) { + p := &Title{ + NotMatches: []common.Regexp{ + common.NewCompiledRegexp(regexp.MustCompile("^(fix|feat|chore): (\\w| )+$")), + }, + Matches: []common.Regexp{}, + } + + runTitleTestCase(t, p, []TitleTestCase{ + { + "empty title", + true, + &pulltest.Context{ + TitleValue: "", + }, + }, + { + "matches pattern", + false, + &pulltest.Context{ + TitleValue: "chore: added tests", + }, + }, + { + "does not match pattern", + true, + &pulltest.Context{ + TitleValue: "changes: added tests", + }, + }, + }) +} + +func TestWithMatchRule(t *testing.T) { + p := &Title{ + NotMatches: []common.Regexp{}, + Matches: []common.Regexp{ + common.NewCompiledRegexp(regexp.MustCompile("^BLOCKED")), + }, + } + + runTitleTestCase(t, p, []TitleTestCase{ + { + "empty title", + false, + &pulltest.Context{ + TitleValue: "", + }, + }, + { + "matches pattern", + true, + &pulltest.Context{ + TitleValue: "BLOCKED: new feature", + }, + }, + { + "does not match pattern", + false, + &pulltest.Context{ + TitleValue: "feat: new feature", + }, + }, + }) +} + +func TestWithMixedRules(t *testing.T) { + p := &Title{ + NotMatches: []common.Regexp{ + common.NewCompiledRegexp(regexp.MustCompile("^(fix|feat|chore): (\\w| )+$")), + common.NewCompiledRegexp(regexp.MustCompile("^BREAKING CHANGE: (\\w| )+$")), + }, + Matches: []common.Regexp{ + common.NewCompiledRegexp(regexp.MustCompile("BLOCKED")), + }, + } + + runTitleTestCase(t, p, []TitleTestCase{ + { + "empty title", + true, + &pulltest.Context{ + TitleValue: "", + }, + }, + { + "matches first pattern in matches list", + false, + &pulltest.Context{ + TitleValue: "fix: fixes failing tests", + }, + }, + { + "matches second pattern in matches list", + false, + &pulltest.Context{ + TitleValue: "BREAKING CHANGE: new api version", + }, + }, + { + "matches pattern in not_matches list", + true, + &pulltest.Context{ + TitleValue: "BLOCKED: not working", + }, + }, + { + "matches pattern in both lists", + true, + &pulltest.Context{ + TitleValue: "BREAKING CHANGE: BLOCKED", + }, + }, + { + "does not match any pattern", + true, + &pulltest.Context{ + TitleValue: "test: adds tests", + }, + }, + }) +} + +type TitleTestCase struct { + name string + expected bool + context pull.Context +} + +func runTitleTestCase(t *testing.T, p Predicate, cases []TitleTestCase) { + ctx := context.Background() + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ok, _, err := p.Evaluate(ctx, tc.context) + if assert.NoError(t, err, "evaluation failed") { + assert.Equal(t, tc.expected, ok, "predicate was not correct") + } + }) + } +} diff --git a/pull/context.go b/pull/context.go index 48a24153..6758841a 100644 --- a/pull/context.go +++ b/pull/context.go @@ -56,6 +56,9 @@ type Context interface { // Number returns the number of the pull request. Number() int + // Title returns the title of the pull request + Title() string + // Author returns the username of the user who opened the pull request. Author() string diff --git a/pull/github.go b/pull/github.go index b6901839..2e0aa4fe 100644 --- a/pull/github.go +++ b/pull/github.go @@ -52,6 +52,7 @@ func (loc Locator) IsComplete() bool { switch { case loc.Value == nil: case loc.Value.Draft == nil: + case loc.Value.GetTitle() == "": case loc.Value.GetCreatedAt().IsZero(): case loc.Value.GetUser().GetLogin() == "": case loc.Value.GetBase().GetRef() == "": @@ -87,6 +88,7 @@ func (loc Locator) toV4(ctx context.Context, client *githubv4.Client) (*v4PullRe } var v4 v4PullRequest + v4.Title = loc.Value.GetTitle() v4.CreatedAt = loc.Value.GetCreatedAt() v4.Author.Login = loc.Value.GetUser().GetLogin() v4.IsCrossRepository = loc.Value.GetHead().GetRepo().GetID() != loc.Value.GetBase().GetRepo().GetID() @@ -163,6 +165,10 @@ func (ghc *GitHubContext) RepositoryName() string { return ghc.repo } +func (ghc *GitHubContext) Title() string { + return ghc.pr.Title +} + func (ghc *GitHubContext) Number() int { return ghc.number } @@ -732,9 +738,10 @@ func backfillPushedAt(commits []*Commit, headSHA string) { } } -// if adding new fields to this struct, modify Locator#toV4() as well +// if adding new fields to this struct, modify Locator#toV4() and Locator#IsComplete() as well type v4PullRequest struct { Author v4Actor + Title string CreatedAt time.Time IsCrossRepository bool diff --git a/pull/github_test.go b/pull/github_test.go index 689c3d2c..d3e3cf34 100644 --- a/pull/github_test.go +++ b/pull/github_test.go @@ -455,6 +455,7 @@ func makeContext(t *testing.T, rp *ResponsePlayer, pr *github.PullRequest) Conte func defaultTestPR() *github.PullRequest { return &github.PullRequest{ + Title: github.String("test title"), Number: github.Int(123), CreatedAt: newTime(time.Date(2020, 9, 30, 17, 42, 10, 0, time.UTC)), Draft: github.Bool(false), diff --git a/pull/pulltest/context.go b/pull/pulltest/context.go index 0bf537e2..d1f0ac2a 100644 --- a/pull/pulltest/context.go +++ b/pull/pulltest/context.go @@ -25,6 +25,7 @@ type Context struct { RepoValue string NumberValue int + TitleValue string AuthorValue string CreatedAtValue time.Time HeadSHAValue string @@ -89,6 +90,10 @@ func (c *Context) Number() int { return 1 } +func (c *Context) Title() string { + return c.TitleValue +} + func (c *Context) Author() string { return c.AuthorValue }