Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support required statuses for Bulldozer to update PRs #307

Merged
merged 15 commits into from
May 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,13 @@ update:
# If true, bulldozer will ignore updating draft pull requests, unless they
# explicitly match a configured trigger condition.
ignore_drafts: false

# "required_statuses" is a list of additional status contexts that must pass
# before bulldozer will update a pull request, unless the pull request
# explicitly matches a configured trigger condition. This is useful if you want
# to require certain statuses to pass before automated updates are made.
required_statuses:
- "policy-bot: develop"
```

#### Remote Configuration
Expand Down
30 changes: 29 additions & 1 deletion bulldozer/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,15 @@ merge:
squash:
body: summarize_commits
delete_after_merge: true
required_statuses: ["Test 1", "Test 2"]

update:
trigger:
labels: ["wip", "update me"]
ignore:
labels: ["do not update"]
ignore_drafts: true
required_statuses: ["Test 3", "Test 4"]
`

actual, err := ParseConfig([]byte(config))
Expand All @@ -57,7 +59,34 @@ update:
Labels: []string{"do not merge"},
CommentSubstrings: []string{"==DO_NOT_MERGE=="},
}, actual.Merge.Ignore)
assert.Equal(t, []string{"Test 1", "Test 2"}, actual.Merge.RequiredStatuses)

assert.Equal(t, *actual.Update.IgnoreDrafts, true)
assert.Equal(t, []string{"Test 3", "Test 4"}, actual.Update.RequiredStatuses)
})

t.Run("parseDefaults", func(t *testing.T) {
config := `
version: 1

merge:
method: squash
options:
squash:
body: summarize_commits
`

actual, err := ParseConfig([]byte(config))
require.Nil(t, err)

assert.Empty(t, actual.Merge.Trigger)
assert.Empty(t, actual.Merge.Ignore)
assert.Empty(t, actual.Merge.RequiredStatuses)

assert.Empty(t, actual.Update.Trigger)
assert.Empty(t, actual.Update.Ignore)
assert.Nil(t, actual.Update.IgnoreDrafts)
assert.Empty(t, actual.Update.RequiredStatuses)
})

t.Run("parseExisting", func(t *testing.T) {
Expand Down Expand Up @@ -102,7 +131,6 @@ update:
assert.Equal(t, Signals{
Labels: []string{"do not update"},
}, actual.Update.Ignore)
assert.Nil(t, actual.Update.IgnoreDrafts)
})

t.Run("ignoresOldConfig", func(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions bulldozer/config_v1.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ type UpdateConfig struct {

IgnoreDrafts *bool `yaml:"ignore_drafts"`

// Additional status checks that bulldozer should require
// (even if the branch protection settings doesn't require it)
RequiredStatuses []string `yaml:"required_statuses"`

// Blacklist and Whitelist are legacy options that will be disabled in a future v2 format
Blacklist Signals `yaml:"blacklist"`
Whitelist Signals `yaml:"whitelist"`
Expand Down
69 changes: 62 additions & 7 deletions bulldozer/evaluate.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,34 +102,34 @@ func ShouldMergePR(ctx context.Context, pullCtx pull.Context, mergeConfig MergeC
if mergeConfig.Ignore.Enabled() {
ignored, reason, err := IsPRIgnored(ctx, pullCtx, mergeConfig.Ignore)
if err != nil {
return false, errors.Wrap(err, "failed to determine if pull request is ignored")
return false, errors.Wrap(err, "failed to determine if pull request is ignored for merge")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying/deduplicating these messages!

}
if ignored {
logger.Debug().Msgf("%s is deemed not mergeable because ignoring is enabled and %s", pullCtx.Locator(), reason)
return false, nil
}
} else {
logger.Debug().Msg("ignoring is not enabled")
logger.Debug().Msg("ignoring for merge is not enabled")
}

if mergeConfig.Trigger.Enabled() {
triggered, reason, err := IsPRTriggered(ctx, pullCtx, mergeConfig.Trigger)
if err != nil {
return false, errors.Wrap(err, "failed to determine if pull request is triggered")
return false, errors.Wrap(err, "failed to determine if pull request is triggered for merge")
}
if !triggered {
logger.Debug().Msgf("%s is deemed not mergeable because triggering is enabled and no trigger signal detected", pullCtx.Locator())
return false, nil
}

logger.Debug().Msgf("%s is triggered because triggering is enabled and %s", pullCtx.Locator(), reason)
logger.Debug().Msgf("%s is triggered for merge because triggering is enabled and %s", pullCtx.Locator(), reason)
} else {
logger.Debug().Msg("triggering is not enabled")
logger.Debug().Msg("triggering for merge is not enabled")
}

requiredStatuses, err := pullCtx.RequiredStatuses(ctx)
if err != nil {
return false, errors.Wrap(err, "failed to determine required Github status checks")
return false, errors.Wrap(err, "failed to determine required Github status checks for merge")
}
requiredStatuses = append(requiredStatuses, mergeConfig.RequiredStatuses...)

Expand All @@ -140,7 +140,7 @@ func ShouldMergePR(ctx context.Context, pullCtx pull.Context, mergeConfig MergeC

successStatuses, err := pullCtx.CurrentSuccessStatuses(ctx)
if err != nil {
return false, errors.Wrap(err, "failed to determine currently successful status checks")
return false, errors.Wrap(err, "failed to determine currently successful status checks for merge")
}

unsatisfiedStatuses := statusSetDifference(requiredStatuses, successStatuses)
Expand All @@ -152,3 +152,58 @@ func ShouldMergePR(ctx context.Context, pullCtx pull.Context, mergeConfig MergeC
// Ignore required reviews and try a merge (which may fail with a 4XX).
return true, nil
}

func ShouldUpdatePR(ctx context.Context, pullCtx pull.Context, updateConfig UpdateConfig) (bool, error) {
logger := zerolog.Ctx(ctx)

if !updateConfig.Ignore.Enabled() && !updateConfig.Trigger.Enabled() && updateConfig.IgnoreDrafts == nil && len(updateConfig.RequiredStatuses) == 0 {
return false, nil
}

if updateConfig.Ignore.Enabled() {
ignored, reason, err := IsPRIgnored(ctx, pullCtx, updateConfig.Ignore)
if err != nil {
return false, errors.Wrap(err, "failed to determine if pull request is ignored for update")
}
if ignored {
logger.Debug().Msgf("%s is deemed not updateable because ignoring is enabled and %s", pullCtx.Locator(), reason)
return false, nil
}
}

if updateConfig.Trigger.Enabled() {
triggered, reason, err := IsPRTriggered(ctx, pullCtx, updateConfig.Trigger)
if err != nil {
return false, errors.Wrap(err, "failed to determine if pull request is triggered for update")
}
if !triggered {
logger.Debug().Msgf("%s is deemed not updateable because triggering is enabled and no trigger signal detected", pullCtx.Locator())
return false, nil
}

logger.Debug().Msgf("%s is triggered for update because triggering is enabled and %s", pullCtx.Locator(), reason)
return true, nil
}

if updateConfig.IgnoreDrafts != nil && *updateConfig.IgnoreDrafts && pullCtx.IsDraft(ctx) {
logger.Debug().Msgf("%s is deemed not updateable because PR is in a draft state", pullCtx.Locator())
return false, nil
}

requiredStatuses := updateConfig.RequiredStatuses

if len(requiredStatuses) > 0 {
bluekeyes marked this conversation as resolved.
Show resolved Hide resolved
successStatuses, err := pullCtx.CurrentSuccessStatuses(ctx)
if err != nil {
return false, errors.Wrap(err, "failed to determine currently successful status checks for update")
}

unsatisfiedStatuses := statusSetDifference(requiredStatuses, successStatuses)
if len(unsatisfiedStatuses) > 0 {
logger.Debug().Msgf("%s is deemed not updateable because of unfulfilled status checks: [%s]", pullCtx.Locator(), strings.Join(unsatisfiedStatuses, ","))
return false, nil
}
}

return true, nil
}
Loading