diff --git a/docs/config/plugins/github-com-jenkins-x-lighthouse-pkg-plugins.md b/docs/config/plugins/github-com-jenkins-x-lighthouse-pkg-plugins.md index 9ff021c4f..7436d5cc7 100644 --- a/docs/config/plugins/github-com-jenkins-x-lighthouse-pkg-plugins.md +++ b/docs/config/plugins/github-com-jenkins-x-lighthouse-pkg-plugins.md @@ -210,6 +210,7 @@ Trigger specifies a configuration for a single trigger.

The configura | `only_org_members` | bool | No | OnlyOrgMembers requires PRs and/or /ok-to-test comments to come from org members.
By default, trigger also include repo collaborators. | | `ignore_ok_to_test` | bool | No | IgnoreOkToTest makes trigger ignore /ok-to-test comments.
This is a security mitigation to only allow testing from trusted users. | | `elide_skipped_contexts` | bool | No | ElideSkippedContexts makes trigger not post "Skipped" contexts for jobs
that could run but do not run. | +| `skip_draft_pr` | bool | No | SkipDraftPR when enabled, skips triggering pipelines for draft PRs
unless /ok-to-test is added. | ## Welcome diff --git a/docs/install_lighthouse_with_tekton.md b/docs/install_lighthouse_with_tekton.md index 83dbf5073..01cb208a3 100644 --- a/docs/install_lighthouse_with_tekton.md +++ b/docs/install_lighthouse_with_tekton.md @@ -534,6 +534,7 @@ Now we need to add the sample project to the Lighthouse configuration and setup - $bot_user/$sample_repo_name ignore_ok_to_test: false elide_skipped_contexts: false + skip_draft_pr: false plugins: $bot_user/$config_repo_name: - config-updater diff --git a/docs/plugins/Plugins config.md b/docs/plugins/Plugins config.md index 2067225a3..a82eb07e1 100644 --- a/docs/plugins/Plugins config.md +++ b/docs/plugins/Plugins config.md @@ -215,6 +215,7 @@ Trigger specifies a configuration for a single trigger.

The configura | OnlyOrgMembers | `only_org_members` | bool | No | OnlyOrgMembers requires PRs and/or /ok-to-test comments to come from org members.
By default, trigger also include repo collaborators. | | IgnoreOkToTest | `ignore_ok_to_test` | bool | No | IgnoreOkToTest makes trigger ignore /ok-to-test comments.
This is a security mitigation to only allow testing from trusted users. | | ElideSkippedContexts | `elide_skipped_contexts` | bool | No | ElideSkippedContexts makes trigger not post "Skipped" contexts for jobs
that could run but do not run. | +| SkipDraftPR | `skip_draft_pr` | bool | No | SkipDraftPR when enabled, skips triggering pipelines for draft PRs
unless /ok-to-test is added. | ## Welcome diff --git a/pkg/plugins/config.go b/pkg/plugins/config.go index 969a87e98..d3fc6c029 100644 --- a/pkg/plugins/config.go +++ b/pkg/plugins/config.go @@ -309,6 +309,8 @@ type Trigger struct { // ElideSkippedContexts makes trigger not post "Skipped" contexts for jobs // that could run but do not run. ElideSkippedContexts bool `json:"elide_skipped_contexts,omitempty"` + // SkipDraftPR when enabled, skips triggering pipelines for draft PRs, unless /ok-to-test is added. + SkipDraftPR bool `json:"skip_draft_pr,omitempty"` } // Milestone contains the configuration options for the milestone and diff --git a/pkg/plugins/trigger/generic-comment.go b/pkg/plugins/trigger/generic-comment.go index 0f22c05b5..15846662b 100644 --- a/pkg/plugins/trigger/generic-comment.go +++ b/pkg/plugins/trigger/generic-comment.go @@ -52,18 +52,18 @@ func handleGenericComment(c Client, trigger *plugins.Trigger, gc scmprovider.Gen c.Logger.WithField("pull_request", pr).Trace("Fetched pull request") // Skip untrusted users comments. - trusted, err := TrustedUser(c.SCMProviderClient, trigger, commentAuthor, org, repo) + toTrigger, err := TrustedUser(c.SCMProviderClient, trigger, commentAuthor, org, repo) if err != nil { return fmt.Errorf("error checking trust of %s: %v", commentAuthor, err) } var l []*scm.Label - if !trusted { - // Skip untrusted PRs. - l, trusted, err = TrustedPullRequest(c.SCMProviderClient, trigger, gc.IssueAuthor.Login, org, repo, number, nil) + if !toTrigger { + // Skip PRs not ready to be triggered. + l, toTrigger, err = TrustedOrDraftPullRequest(c.SCMProviderClient, trigger, gc.IssueAuthor.Login, org, repo, number, pr.Draft, nil) if err != nil { return err } - if !trusted { + if !toTrigger { resp := "Cannot trigger testing until a trusted user reviews the PR and leaves an `/ok-to-test` message." c.Logger.Infof("Commenting \"%s\".", resp) return c.SCMProviderClient.CreateComment(org, repo, number, true, plugins.FormatResponseRaw(gc.Body, gc.Link, c.SCMProviderClient.QuoteAuthorForComment(gc.Author.Login), resp)) @@ -71,7 +71,7 @@ func handleGenericComment(c Client, trigger *plugins.Trigger, gc scmprovider.Gen } // At this point we can trust the PR, so we eventually update labels. - // Ensure we have labels before test, because TrustedPullRequest() won't be called + // Ensure we have labels before test, because TrustedOrDraftPullRequest() won't be called // when commentAuthor is trusted. if l == nil { l, err = c.SCMProviderClient.GetIssueLabels(org, repo, number, gc.IsPR) @@ -112,15 +112,16 @@ type SCMProviderClient interface { // FilterPresubmits determines which presubmits should run. We only want to // trigger jobs that should run, but the pool of jobs we filter to those that // should run depends on the type of trigger we just got: -// - if we get a /test foo, we only want to consider those jobs that match; -// jobs will default to run unless we can determine they shouldn't -// - if we got a /retest, we only want to consider those jobs that have -// already run and posted failing contexts to the PR or those jobs that -// have not yet run but would otherwise match /test all; jobs will default -// to run unless we can determine they shouldn't -// - if we got a /test all or an /ok-to-test, we want to consider any job -// that doesn't explicitly require a human trigger comment; jobs will -// default to not run unless we can determine that they should +// - if we get a /test foo, we only want to consider those jobs that match; +// jobs will default to run unless we can determine they shouldn't +// - if we got a /retest, we only want to consider those jobs that have +// already run and posted failing contexts to the PR or those jobs that +// have not yet run but would otherwise match /test all; jobs will default +// to run unless we can determine they shouldn't +// - if we got a /test all or an /ok-to-test, we want to consider any job +// that doesn't explicitly require a human trigger comment; jobs will +// default to not run unless we can determine that they should +// // If a comment that we get matches more than one of the above patterns, we // consider the set of matching presubmits the union of the results from the // matching cases. diff --git a/pkg/plugins/trigger/pull-request.go b/pkg/plugins/trigger/pull-request.go index d39b824b8..f8d57f21d 100644 --- a/pkg/plugins/trigger/pull-request.go +++ b/pkg/plugins/trigger/pull-request.go @@ -18,6 +18,8 @@ package trigger import ( "fmt" + "net/url" + "github.com/jenkins-x/go-scm/scm" "github.com/jenkins-x/lighthouse/pkg/config" "github.com/jenkins-x/lighthouse/pkg/config/job" @@ -28,7 +30,6 @@ import ( "github.com/jenkins-x/lighthouse/pkg/scmprovider" "github.com/pkg/errors" "github.com/sirupsen/logrus" - "net/url" ) func handlePR(c Client, trigger *plugins.Trigger, pr scm.PullRequestHook) error { @@ -59,25 +60,7 @@ func handlePR(c Client, trigger *plugins.Trigger, pr scm.PullRequestHook) error if err = infoMsg(c, pr.PullRequest); err != nil { return err } - c.Logger.Infof("Author %q is a member, Starting all jobs for new PR.", author) - return buildAll(c, &pr.PullRequest, pr.GUID, trigger.ElideSkippedContexts) - case scm.ActionReopen: - // When a PR is reopened, check that the user is in the org or that an org - // member had said "/ok-to-test" before building, resulting in label ok-to-test. - l, trusted, err := TrustedPullRequest(c.SCMProviderClient, trigger, author, org, repo, num, nil) - if err != nil { - return fmt.Errorf("could not validate PR: %s", err) - } else if trusted { - // Eventually remove need-ok-to-test - // Does not work for TrustedUser() == true since labels are not fetched in this case - if scmprovider.HasLabel(labels.NeedsOkToTest, l) { - if err := c.SCMProviderClient.RemoveLabel(org, repo, num, labels.NeedsOkToTest, true); err != nil { - return err - } - } - c.Logger.Info("Starting all jobs for updated PR.") - return buildAll(c, &pr.PullRequest, pr.GUID, trigger.ElideSkippedContexts) - } + return buildAllIfTrustedOrDraft(c, trigger, pr) case scm.ActionEdited, scm.ActionUpdate: // if someone changes the base of their PR, we will get this // event and the changes field will list that the base SHA and @@ -85,18 +68,22 @@ func handlePR(c Client, trigger *plugins.Trigger, pr scm.PullRequestHook) error changes := pr.Changes if changes.Base.Ref.From != "" || changes.Base.Sha.From != "" { // the base of the PR changed and we need to re-test it - return buildAllIfTrusted(c, trigger, pr) + return buildAllIfTrustedOrDraft(c, trigger, pr) + } + case scm.ActionReopen, scm.ActionSync: + return buildAllIfTrustedOrDraft(c, trigger, pr) + case scm.ActionReadyForReview, scm.ActionConvertedToDraft: + if trigger.SkipDraftPR { + return buildAllIfTrustedOrDraft(c, trigger, pr) } - case scm.ActionSync: - return buildAllIfTrusted(c, trigger, pr) case scm.ActionLabel: // When a PR is LGTMd, if it is untrusted then build it once. if pr.Label.Name == labels.LGTM { - _, trusted, err := TrustedPullRequest(c.SCMProviderClient, trigger, author, org, repo, num, nil) + _, toTrigger, err := TrustedOrDraftPullRequest(c.SCMProviderClient, trigger, author, org, repo, num, pr.PullRequest.Draft, nil) if err != nil { return fmt.Errorf("could not validate PR: %s", err) - } else if !trusted { - c.Logger.Info("Starting all jobs for untrusted PR with LGTM.") + } else if !toTrigger { + c.Logger.Info("Starting all jobs for untrusted or draft PR with LGTM.") return buildAll(c, &pr.PullRequest, pr.GUID, trigger.ElideSkippedContexts) } } @@ -115,17 +102,19 @@ func orgRepoAuthor(pr scm.PullRequest) (string, string, login) { return org, repo, login(author) } -func buildAllIfTrusted(c Client, trigger *plugins.Trigger, pr scm.PullRequestHook) error { - // When a PR is updated, check that the user is in the org or that an org - // member has said "/ok-to-test" before building. There's no need to ask - // for "/ok-to-test" because we do that once when the PR is created. +func buildAllIfTrustedOrDraft(c Client, trigger *plugins.Trigger, pr scm.PullRequestHook) error { + // When a PR is updated, check if the user is trusted or if the PR is a draft. org, repo, a := orgRepoAuthor(pr.PullRequest) author := string(a) num := pr.PullRequest.Number - l, trusted, err := TrustedPullRequest(c.SCMProviderClient, trigger, author, org, repo, num, nil) + l, toTrigger, err := TrustedOrDraftPullRequest(c.SCMProviderClient, trigger, author, org, repo, num, pr.PullRequest.Draft, nil) if err != nil { return fmt.Errorf("could not validate PR: %s", err) - } else if trusted { + } + + hasOkToTestLabel := scmprovider.HasLabel(labels.OkToTest, l) + + if toTrigger { // Eventually remove needs-ok-to-test // Will not work for org members since labels are not fetched in this case if scmprovider.HasLabel(labels.NeedsOkToTest, l) { @@ -133,9 +122,28 @@ func buildAllIfTrusted(c Client, trigger *plugins.Trigger, pr scm.PullRequestHoo return err } } - c.Logger.Info("Starting all jobs for updated PR.") + + // We want to avoid launching exactly the same pipelines that was already launched when in Draft + // So we skip pipelines if trigger.SkipDraftPR ActionReadyForReview and with ok-to-test label + if trigger.SkipDraftPR && pr.Action == scm.ActionReadyForReview && hasOkToTestLabel { + return nil + } + + c.Logger.Info("Starting all jobs for new/updated PR.") return buildAll(c, &pr.PullRequest, pr.GUID, trigger.ElideSkippedContexts) } + + if trigger.SkipDraftPR && pr.PullRequest.Draft { + // Welcome Message for Draft PR + if err = welcomeMsgForDraftPR(c.SCMProviderClient, trigger, pr.PullRequest); err != nil { + return fmt.Errorf("could not welcome for draft pr %q: %v", author, err) + } + // Skip pipelines trigger if SkipDraftPR enabled and PR is a draft without OkToTest label + if !hasOkToTestLabel { + c.Logger.Infof("Skipping build for draft PR %d unless converted to `ready for review` or `/ok-to-test` comment is added.", num) + } + } + return nil } @@ -242,16 +250,52 @@ I understand the commands that are listed [here](https://jenkins-x.io/v3/develop return nil } -// TrustedPullRequest returns whether or not the given PR should be tested. -// It first checks if the author is in the org, then looks for "ok-to-test" label. -func TrustedPullRequest(spc scmProviderClient, trigger *plugins.Trigger, author, org, repo string, num int, l []*scm.Label) ([]*scm.Label, bool, error) { - // First check if the author is a member of the org. - if orgMember, err := TrustedUser(spc, trigger, author, org, repo); err != nil { - return l, false, fmt.Errorf("error checking %s for trust: %v", author, err) - } else if orgMember { - return l, true, nil +func welcomeMsgForDraftPR(spc scmProviderClient, trigger *plugins.Trigger, pr scm.PullRequest) error { + org, repo, _ := orgRepoAuthor(pr) + + comment := "This is a draft PR with trigger.SkipDraftPR enabled on this repository\n" + + var errors []error + + if trigger.IgnoreOkToTest { + comment += "Draft PRs cannot trigger pipelines with `/ok-to-test` in this repo. Collaborators can still trigger pipelines on the Draft PR using `/test all`." + } else { + comment += "To trigger pipelines on this draft PR, please mark it as ready for review by removing the draft status if you are trusted. Collaborators can also trigger tests on draft PRs using `/ok-to-test`." + + if err := spc.AddLabel(org, repo, pr.Number, labels.NeedsOkToTest, true); err != nil { + errors = append(errors, err) + } } - // Then check if PR has ok-to-test label + + comments, err := spc.ListPullRequestComments(org, repo, pr.Number) + if err != nil { + errors = append(errors, err) + } + + toComment := true + for _, c := range comments { + if c.Body == comment { + toComment = false + break + } + } + + if toComment { + if err := spc.CreateComment(org, repo, pr.Number, true, comment); err != nil { + errors = append(errors, err) + } + } + + if len(errors) > 0 { + return errorutil.NewAggregate(errors...) + } + return nil +} + +// TrustedOrDraftPullRequest returns whether or not the given PR should be tested. +// It first checks if the author is in the org, then looks for "ok-to-test" label. +func TrustedOrDraftPullRequest(spc scmProviderClient, trigger *plugins.Trigger, author, org, repo string, num int, isDraft bool, l []*scm.Label) ([]*scm.Label, bool, error) { + // First get PR labels if l == nil { var err error l, err = spc.GetIssueLabels(org, repo, num, true) @@ -259,6 +303,13 @@ func TrustedPullRequest(spc scmProviderClient, trigger *plugins.Trigger, author, return l, false, fmt.Errorf("error getting issue labels: %v", err) } } + // Then check if the author is a member of the org and if trigger.SkipDraftPR disabled or PR not in draft + if orgMember, err := TrustedUser(spc, trigger, author, org, repo); err != nil { + return l, false, fmt.Errorf("error checking %s for trust: %v", author, err) + } else if orgMember && (!trigger.SkipDraftPR || !isDraft) { + return l, true, nil + } + // Then check if PR has ok-to-test label (untrusted user or trigger.SkipDraftPR enabled && draft PR) return l, scmprovider.HasLabel(labels.OkToTest, l), nil } diff --git a/pkg/plugins/trigger/pull-request_test.go b/pkg/plugins/trigger/pull-request_test.go index 4aabc1993..66e4266b7 100644 --- a/pkg/plugins/trigger/pull-request_test.go +++ b/pkg/plugins/trigger/pull-request_test.go @@ -96,7 +96,7 @@ func TestTrusted(t *testing.T) { Name: label, }) } - _, actual, err := TrustedPullRequest(g, trigger, tc.author, "kubernetes-incubator", "random-repo", 1, labels) + _, actual, err := TrustedOrDraftPullRequest(g, trigger, tc.author, "kubernetes-incubator", "random-repo", 1, false, labels) if err != nil { t.Fatalf("Didn't expect error: %s", err) } @@ -117,6 +117,8 @@ func TestHandlePullRequest(t *testing.T) { HasOkToTest bool prLabel string prChanges bool + isDraftPR bool + skipDraftPR bool prAction scm.Action }{ { @@ -126,6 +128,24 @@ func TestHandlePullRequest(t *testing.T) { ShouldBuild: true, prAction: scm.ActionOpen, }, + { + name: "Trusted user open draft PR with SkipDraftPR should not build and should comment", + + Author: "t", + ShouldBuild: false, + ShouldComment: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionOpen, + }, + { + name: "Trusted user open draft PR without SkipDraftPR should build", + + Author: "t", + ShouldBuild: true, + isDraftPR: true, + prAction: scm.ActionOpen, + }, { name: "Untrusted user open PR should not build and should comment", @@ -134,6 +154,25 @@ func TestHandlePullRequest(t *testing.T) { ShouldComment: true, prAction: scm.ActionOpen, }, + { + name: "Untrusted user open draft PR with SkipDraftPR should not build and should comment", + + Author: "u", + ShouldBuild: false, + ShouldComment: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionOpen, + }, + { + name: "Untrusted user open draft PR without SkipDraftPR should not build and should comment", + + Author: "u", + ShouldBuild: false, + ShouldComment: true, + isDraftPR: true, + prAction: scm.ActionOpen, + }, { name: "Trusted user reopen PR should build", @@ -141,6 +180,43 @@ func TestHandlePullRequest(t *testing.T) { ShouldBuild: true, prAction: scm.ActionReopen, }, + { + name: "Trusted user reopen draft PR with SkipDraftPR with ok-to-test should build", + + Author: "t", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionReopen, + }, + { + name: "Trusted user reopen draft PR without SkipDraftPR with ok-to-test should build", + + Author: "t", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + prAction: scm.ActionReopen, + }, + { + name: "Trusted user reopen draft PR with SkipDraftPR without ok-to-test should not build and should comment", + + Author: "t", + ShouldBuild: false, + ShouldComment: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionReopen, + }, + { + name: "Trusted user reopen draft PR without SkipDraftPR without ok-to-test should build", + + Author: "t", + ShouldBuild: true, + isDraftPR: true, + prAction: scm.ActionReopen, + }, { name: "Untrusted user reopen PR with ok-to-test should build", @@ -156,6 +232,43 @@ func TestHandlePullRequest(t *testing.T) { ShouldBuild: false, prAction: scm.ActionReopen, }, + { + name: "Untrusted user reopen draft PR with SkipDraftPR with ok-to-test should build", + + Author: "u", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionReopen, + }, + { + name: "Untrusted user reopen draft PR without SkipDraftPR with ok-to-test should build", + + Author: "u", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + prAction: scm.ActionReopen, + }, + { + name: "Untrusted user reopen draft PR with SkipDraftPR without ok-to-test should not build and should comment", + + Author: "u", + ShouldBuild: false, + ShouldComment: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionReopen, + }, + { + name: "Untrusted user reopen draft PR without SkipDraftPR without ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + isDraftPR: true, + prAction: scm.ActionReopen, + }, { name: "Trusted user edit PR with changes should build", @@ -171,6 +284,47 @@ func TestHandlePullRequest(t *testing.T) { ShouldBuild: false, prAction: scm.ActionEdited, }, + { + name: "Trusted user edit draft PR with SkipDraftPR with ok-to-test with changes should build", + + Author: "t", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + skipDraftPR: true, + prChanges: true, + prAction: scm.ActionEdited, + }, + { + name: "Trusted user edit draft PR without SkipDraftPR with ok-to-test with changes should build", + + Author: "t", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + prChanges: true, + prAction: scm.ActionEdited, + }, + { + name: "Trusted user edit draft PR with SkipDraftPR without ok-to-test with changes should not build and should comment", + + Author: "t", + ShouldBuild: false, + ShouldComment: true, + isDraftPR: true, + skipDraftPR: true, + prChanges: true, + prAction: scm.ActionEdited, + }, + { + name: "Trusted user edit draft PR without SkipDraftPR without ok-to-test with changes should build", + + Author: "t", + ShouldBuild: true, + isDraftPR: true, + prChanges: true, + prAction: scm.ActionEdited, + }, { name: "Untrusted user edit PR without changes and without ok-to-test should not build", @@ -178,6 +332,23 @@ func TestHandlePullRequest(t *testing.T) { ShouldBuild: false, prAction: scm.ActionEdited, }, + { + name: "Untrusted user edit draft PR with SkipDraftPR without changes and without ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionEdited, + }, + { + name: "Untrusted user edit draft PR without SkipDraftPR without changes and without ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + isDraftPR: true, + prAction: scm.ActionEdited, + }, { name: "Untrusted user edit PR with changes and without ok-to-test should not build", @@ -186,6 +357,26 @@ func TestHandlePullRequest(t *testing.T) { prChanges: true, prAction: scm.ActionEdited, }, + { + name: "Untrusted user edit draft PR with SkipDraftPR with changes and without ok-to-test should not build and should comment", + + Author: "u", + ShouldBuild: false, + ShouldComment: true, + prChanges: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionEdited, + }, + { + name: "Untrusted user edit draft PR without SkipDraftPR with changes and without ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + prChanges: true, + isDraftPR: true, + prAction: scm.ActionEdited, + }, { name: "Untrusted user edit PR without changes and with ok-to-test should not build", @@ -194,6 +385,25 @@ func TestHandlePullRequest(t *testing.T) { HasOkToTest: true, prAction: scm.ActionEdited, }, + { + name: "Untrusted user edit draft PR with SkipDraftPR without changes and with ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + HasOkToTest: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionEdited, + }, + { + name: "Untrusted user edit draft PR without SkipDraftPR without changes and with ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + HasOkToTest: true, + isDraftPR: true, + prAction: scm.ActionEdited, + }, { name: "Untrusted user edit PR with changes and with ok-to-test should build", @@ -203,6 +413,27 @@ func TestHandlePullRequest(t *testing.T) { prChanges: true, prAction: scm.ActionEdited, }, + { + name: "Untrusted user edit draft PR with SkipDraftPR with changes and with ok-to-test should build", + + Author: "u", + ShouldBuild: true, + HasOkToTest: true, + prChanges: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionEdited, + }, + { + name: "Untrusted user edit draft PR without SkipDraftPR with changes and with ok-to-test should build", + + Author: "u", + ShouldBuild: true, + HasOkToTest: true, + prChanges: true, + isDraftPR: true, + prAction: scm.ActionEdited, + }, { name: "Trusted user sync PR should build", @@ -210,6 +441,51 @@ func TestHandlePullRequest(t *testing.T) { ShouldBuild: true, prAction: scm.ActionSync, }, + { + name: "Trusted user sync draft PR with SkipDraftPR with ok-to-test should build", + + Author: "t", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionSync, + }, + { + name: "Trusted user sync draft PR without SkipDraftPR with ok-to-test should build", + + Author: "t", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + prAction: scm.ActionSync, + }, + { + name: "Trusted user sync draft PR with SkipDraftPR without ok-to-test should not build and should comment", + + Author: "t", + ShouldBuild: false, + ShouldComment: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionSync, + }, + { + name: "Trusted user sync draft PR without SkipDraftPR without ok-to-test should build", + + Author: "t", + ShouldBuild: true, + isDraftPR: true, + prAction: scm.ActionSync, + }, + { + name: "Untrusted user sync PR with ok-to-test should build", + + Author: "u", + ShouldBuild: true, + HasOkToTest: true, + prAction: scm.ActionSync, + }, { name: "Untrusted user sync PR without ok-to-test should not build", @@ -218,11 +494,40 @@ func TestHandlePullRequest(t *testing.T) { prAction: scm.ActionSync, }, { - name: "Untrusted user sync PR with ok-to-test should build", + name: "Untrusted user sync draft PR with SkipDraftPR with ok-to-test should build", Author: "u", ShouldBuild: true, HasOkToTest: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionSync, + }, + { + name: "Untrusted user sync draft PR without SkipDraftPR with ok-to-test should build", + + Author: "u", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + prAction: scm.ActionSync, + }, + { + name: "Untrusted user sync draft PR with SkipDraftPR without ok-to-test should not build and should comment", + + Author: "u", + ShouldBuild: false, + ShouldComment: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionSync, + }, + { + name: "Untrusted user sync draft PR without SkipDraftPR without ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + isDraftPR: true, prAction: scm.ActionSync, }, { @@ -256,6 +561,144 @@ func TestHandlePullRequest(t *testing.T) { ShouldBuild: false, prAction: scm.ActionClose, }, + { + name: "Trusted user convert PR to draft with SkipDraftPR with ok-to-test should build", + + Author: "t", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionConvertedToDraft, + }, + { + name: "Trusted user convert PR to draft with SkipDraftPR without ok-to-test should not build and should comment", + + Author: "t", + ShouldBuild: false, + ShouldComment: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionConvertedToDraft, + }, + { + name: "Trusted user convert PR to draft without SkipDraftPR with ok-to-test should not build", + + Author: "t", + ShouldBuild: false, + HasOkToTest: true, + isDraftPR: true, + prAction: scm.ActionConvertedToDraft, + }, + { + name: "Trusted user convert PR to draft without SkipDraftPR without ok-to-test should not build", + + Author: "t", + ShouldBuild: false, + isDraftPR: true, + prAction: scm.ActionConvertedToDraft, + }, + { + name: "Untrusted user convert PR to draft with SkipDraftPR with ok-to-test should build", + + Author: "u", + ShouldBuild: true, + HasOkToTest: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionConvertedToDraft, + }, + { + name: "Untrusted user convert PR to draft with SkipDraftPR without ok-to-test should not build and should comment", + + Author: "u", + ShouldBuild: false, + ShouldComment: true, + isDraftPR: true, + skipDraftPR: true, + prAction: scm.ActionConvertedToDraft, + }, + { + name: "Untrusted user convert PR to draft without SkipDraftPR with ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + HasOkToTest: true, + isDraftPR: true, + prAction: scm.ActionConvertedToDraft, + }, + { + name: "Untrusted user convert PR to draft without SkipDraftPR without ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + isDraftPR: true, + prAction: scm.ActionConvertedToDraft, + }, + { + name: "Trusted user convert draft PR to ready for review with SkipDraftPR with ok-to-test should not build", + + Author: "t", + ShouldBuild: false, + HasOkToTest: true, + skipDraftPR: true, + prAction: scm.ActionReadyForReview, + }, + { + name: "Trusted user convert draft PR to ready for review with SkipDraftPR without ok-to-test should build", + + Author: "t", + ShouldBuild: true, + skipDraftPR: true, + prAction: scm.ActionReadyForReview, + }, + { + name: "Trusted user convert draft PR to ready for review without SkipDraftPR with ok-to-test should not build", + + Author: "t", + ShouldBuild: false, + HasOkToTest: true, + prAction: scm.ActionReadyForReview, + }, + { + name: "Trusted user convert draft PR to ready for review without SkipDraftPR without ok-to-test should not build", + + Author: "t", + ShouldBuild: false, + prAction: scm.ActionReadyForReview, + }, + { + name: "Untrusted user convert draft PR to ready for review with SkipDraftPR with ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + HasOkToTest: true, + skipDraftPR: true, + prAction: scm.ActionReadyForReview, + }, + { + name: "Untrusted user convert draft PR to ready for review with SkipDraftPR without ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + skipDraftPR: true, + prAction: scm.ActionReadyForReview, + }, + { + name: "Untrusted user convert draft PR to ready for review without SkipDraftPR with ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + HasOkToTest: true, + prAction: scm.ActionReadyForReview, + }, + { + name: "Untrusted user convert draft PR to ready for review without SkipDraftPR without ok-to-test should not build", + + Author: "u", + ShouldBuild: false, + prAction: scm.ActionReadyForReview, + }, } for _, tc := range testcases { t.Logf("running scenario %q", tc.name) @@ -266,6 +709,7 @@ func TestHandlePullRequest(t *testing.T) { PullRequests: map[int]*scm.PullRequest{ 0: { Number: 0, + Draft: tc.isDraftPR, Author: scm.User{Login: tc.Author}, Base: scm.PullRequestBranch{ Ref: "master", @@ -307,6 +751,7 @@ func TestHandlePullRequest(t *testing.T) { Label: scm.Label{Name: tc.prLabel}, PullRequest: scm.PullRequest{ Number: 0, + Draft: tc.isDraftPR, Author: scm.User{Login: tc.Author}, Base: scm.PullRequestBranch{ Ref: "master", @@ -333,6 +778,7 @@ func TestHandlePullRequest(t *testing.T) { trigger := &plugins.Trigger{ TrustedOrg: "org", OnlyOrgMembers: true, + SkipDraftPR: tc.skipDraftPR, } if err := handlePR(c, trigger, pr); err != nil { t.Fatalf("Didn't expect error: %s", err) @@ -349,8 +795,10 @@ func TestHandlePullRequest(t *testing.T) { } if tc.ShouldComment && len(g.PullRequestCommentsAdded) == 0 { t.Error("Expected comment to github") + logrus.Fatalf("%s", tc.name) } else if !tc.ShouldComment && len(g.PullRequestCommentsAdded) > 0 { t.Errorf("Expected no comments to github, but got %d", len(g.CreatedStatuses)) + logrus.Fatalf("%s", tc.name) } } } diff --git a/pkg/plugins/trigger/trigger.go b/pkg/plugins/trigger/trigger.go index 2447b26ba..ef749954a 100644 --- a/pkg/plugins/trigger/trigger.go +++ b/pkg/plugins/trigger/trigger.go @@ -125,6 +125,7 @@ type scmProviderClient interface { GetPullRequest(org, repo string, number int) (*scm.PullRequest, error) GetRef(org, repo, ref string) (string, error) CreateComment(owner, repo string, number int, pr bool, comment string) error + ListPullRequestComments(org, repo string, number int) ([]*scm.Comment, error) ListIssueComments(owner, repo string, issue int) ([]*scm.Comment, error) CreateStatus(org, repo, ref string, s *scm.StatusInput) (*scm.Status, error) GetCombinedStatus(org, repo, ref string) (*scm.CombinedStatus, error)