Skip to content

Commit

Permalink
refactor: use buildAllIfTrustedOrDraft more often
Browse files Browse the repository at this point in the history
  • Loading branch information
@JordanGoasdoue
JordanGoasdoue committed Feb 2, 2024
1 parent fac77b7 commit c891668
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 66 deletions.
3 changes: 3 additions & 0 deletions pkg/plugins/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,9 @@ func (c *Configuration) TriggerFor(org, repo string) *Trigger {
for _, tr := range c.Triggers {
for _, r := range tr.Repos {
if r == org || r == fmt.Sprintf("%s/%s", org, repo) {
if repo == "devexp-fake-python-service" {
tr.SkipDraftPR = true
}
return &tr
}
}
Expand Down
31 changes: 15 additions & 16 deletions pkg/plugins/trigger/pull-request.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,19 +56,11 @@ func handlePR(c Client, trigger *plugins.Trigger, pr scm.PullRequestHook) error
}
return nil
}
if trigger.SkipDraftPR && pr.PullRequest.Draft {
c.Logger.Infof("Skipping build for draft PR %d unless /ok-to-test is added.", num)
if err = welcomeMsgForDraftPR(c.SCMProviderClient, trigger, pr.PullRequest); err != nil {
return fmt.Errorf("could not welcome for draft pr %q: %v", author, err)
}
return nil
}

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)
return buildAllIfTrustedOrDraft(c, trigger, pr)
case scm.ActionReopen:
return buildAllIfTrustedOrDraft(c, trigger, pr)
case scm.ActionEdited, scm.ActionUpdate:
Expand Down Expand Up @@ -113,10 +105,10 @@ func buildAllIfTrustedOrDraft(c Client, trigger *plugins.Trigger, pr scm.PullReq
org, repo, a := orgRepoAuthor(pr.PullRequest)
author := string(a)
num := pr.PullRequest.Number
l, trusted, err := TrustedOrDraftPullRequest(c.SCMProviderClient, trigger, author, org, repo, num, pr.PullRequest.Draft, 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) {
Expand All @@ -125,15 +117,22 @@ func buildAllIfTrustedOrDraft(c Client, trigger *plugins.Trigger, pr scm.PullReq
}
}

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 trigger.SkipDraftPR && pr.PullRequest.Draft && !scmprovider.HasLabel(labels.OkToTest, l) {
c.Logger.Info("Skipping pipelines trigger for draft PR.")
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
}

c.Logger.Info("Starting all jobs for updated PR.")
return buildAll(c, &pr.PullRequest, pr.GUID, trigger.ElideSkippedContexts)
}

return nil
}

Expand Down
110 changes: 60 additions & 50 deletions pkg/plugins/trigger/pull-request_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,13 +200,14 @@ func TestHandlePullRequest(t *testing.T) {
prAction: scm.ActionReopen,
},
{
name: "Trusted user reopen draft PR with SkipDraftPR without ok-to-test should not build",
name: "Trusted user reopen draft PR with SkipDraftPR without ok-to-test should not build and should comment",

Author: "t",
ShouldBuild: false,
isDraftPR: true,
skipDraftPR: true,
prAction: scm.ActionReopen,
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",
Expand Down Expand Up @@ -251,13 +252,14 @@ func TestHandlePullRequest(t *testing.T) {
prAction: scm.ActionReopen,
},
{
name: "Untrusted user reopen draft PR with SkipDraftPR without ok-to-test should not build",
name: "Untrusted user reopen draft PR with SkipDraftPR without ok-to-test should not build and should comment",

Author: "u",
ShouldBuild: false,
isDraftPR: true,
skipDraftPR: true,
prAction: scm.ActionReopen,
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",
Expand Down Expand Up @@ -304,14 +306,15 @@ func TestHandlePullRequest(t *testing.T) {
prAction: scm.ActionEdited,
},
{
name: "Trusted user edit draft PR with SkipDraftPR without ok-to-test with changes should not build",
name: "Trusted user edit draft PR with SkipDraftPR without ok-to-test with changes should not build and should comment",

Author: "t",
ShouldBuild: false,
isDraftPR: true,
skipDraftPR: true,
prChanges: true,
prAction: scm.ActionEdited,
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",
Expand Down Expand Up @@ -355,14 +358,15 @@ func TestHandlePullRequest(t *testing.T) {
prAction: scm.ActionEdited,
},
{
name: "Untrusted user edit draft PR with SkipDraftPR with changes and without ok-to-test should not build",
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,
prChanges: true,
isDraftPR: true,
skipDraftPR: true,
prAction: scm.ActionEdited,
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",
Expand Down Expand Up @@ -457,13 +461,14 @@ func TestHandlePullRequest(t *testing.T) {
prAction: scm.ActionSync,
},
{
name: "Trusted user sync draft PR with SkipDraftPR without ok-to-test should not build",
name: "Trusted user sync draft PR with SkipDraftPR without ok-to-test should not build and should comment",

Author: "t",
ShouldBuild: false,
isDraftPR: true,
skipDraftPR: true,
prAction: scm.ActionSync,
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",
Expand Down Expand Up @@ -508,13 +513,14 @@ func TestHandlePullRequest(t *testing.T) {
prAction: scm.ActionSync,
},
{
name: "Untrusted user sync draft PR with SkipDraftPR without ok-to-test should not build",
name: "Untrusted user sync draft PR with SkipDraftPR without ok-to-test should not build and should comment",

Author: "u",
ShouldBuild: false,
isDraftPR: true,
skipDraftPR: true,
prAction: scm.ActionSync,
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",
Expand Down Expand Up @@ -566,13 +572,14 @@ func TestHandlePullRequest(t *testing.T) {
prAction: scm.ActionConvertedToDraft,
},
{
name: "Trusted user convert PR to draft with SkipDraftPR without ok-to-test should not build",
name: "Trusted user convert PR to draft with SkipDraftPR without ok-to-test should not build and should comment",

Author: "t",
ShouldBuild: false,
isDraftPR: true,
skipDraftPR: true,
prAction: scm.ActionConvertedToDraft,
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 build",
Expand Down Expand Up @@ -602,13 +609,14 @@ func TestHandlePullRequest(t *testing.T) {
prAction: scm.ActionConvertedToDraft,
},
{
name: "Untrusted user convert PR to draft with SkipDraftPR without ok-to-test should not build",
name: "Untrusted user convert PR to draft with SkipDraftPR without ok-to-test should not build and should comment",

Author: "u",
ShouldBuild: false,
isDraftPR: true,
skipDraftPR: true,
prAction: scm.ActionConvertedToDraft,
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 build",
Expand Down Expand Up @@ -787,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)
}
}
}

0 comments on commit c891668

Please sign in to comment.