feat: allow to skip draft prs unless /ok-to-test is added

docs/config/plugins/github-com-jenkins-x-lighthouse-pkg-plugins.md

@@ -210,6 +210,7 @@ Trigger specifies a configuration for a single trigger.<br /><br />The configura
 | `only_org_members` | bool | No | OnlyOrgMembers requires PRs and/or /ok-to-test comments to come from org members.<br />By default, trigger also include repo collaborators. |
 | `ignore_ok_to_test` | bool | No | IgnoreOkToTest makes trigger ignore /ok-to-test comments.<br />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<br />that could run but do not run. |
+| `skip_draft_pr` | bool | No | SkipDraftPR when enabled, skips triggering pipelines for draft PRs<br />unless /ok-to-test is added. |
 
 ## Welcome
 

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

docs/plugins/Plugins config.md

@@ -215,6 +215,7 @@ Trigger specifies a configuration for a single trigger.<br /><br />The configura
 | OnlyOrgMembers | `only_org_members` | bool | No | OnlyOrgMembers requires PRs and/or /ok-to-test comments to come from org members.<br />By default, trigger also include repo collaborators. |
 | IgnoreOkToTest | `ignore_ok_to_test` | bool | No | IgnoreOkToTest makes trigger ignore /ok-to-test comments.<br />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<br />that could run but do not run. |
+| SkipDraftPR | `skip_draft_pr` | bool | No | SkipDraftPR when enabled, skips triggering pipelines for draft PRs<br />unless /ok-to-test is added. |
 
 ## Welcome
 

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

pkg/plugins/trigger/generic-comment.go

@@ -52,26 +52,26 @@ 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))
 		}
 	}
 
 	// 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.

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,44 +60,30 @@ 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
 		// ref changes so we can detect such a case and retrigger tests
 		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,27 +102,38 @@ 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 {
+	} else 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) {
 			if err := c.SCMProviderClient.RemoveLabel(org, repo, num, labels.NeedsOkToTest, true); err != nil {
 				return err
 			}
 		}
-		c.Logger.Info("Starting all jobs for updated PR.")
+
+		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 !scmprovider.HasLabel(labels.OkToTest, l) {
+			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,23 +240,66 @@ 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)
+		}
+	}
+
+	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)
+		}
 	}
-	// Then check if PR has ok-to-test label
+
+	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)
 		if err != nil {
 			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
 }