Skip to content

Commit

Permalink
Replace use of whitelist/blacklist with trigger/ignore (#196)
Browse files Browse the repository at this point in the history
  • Loading branch information
asvoboda authored Jul 28, 2020
1 parent 36145b5 commit 88d5aed
Show file tree
Hide file tree
Showing 11 changed files with 293 additions and 112 deletions.
54 changes: 27 additions & 27 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ status checks are successful and required reviews are provided.

Additionally, `bulldozer` can:

- Only merge pull requests that match a whitelist condition, like having a
- Only merge pull requests that match certain conditions, like having a
specific label or comment
- Ignore pull requests that match a blacklist condition, like having a specific
- Ignore pull requests that match certain conditions, like having a specific
label or comment
- Automatically keep pull request branches up-to-date by merging in the target
branch
Expand Down Expand Up @@ -47,8 +47,8 @@ including required status checks and required reviews, are respected. It also
means that you _must_ enable branch protection to prevent `bulldozer` from
immediately merging every pull request.

Only pull requests matching the whitelist conditions (or _not_ matching the
blacklist conditions) are considered for merging. `bulldozer` is event-driven,
Only pull requests matching the trigger conditions (or _not_ matching
ignore conditions) are considered for merging. `bulldozer` is event-driven,
which means it will usually merge a pull request within a few seconds of the
pull request satisfying all preconditions.

Expand All @@ -74,33 +74,33 @@ version: 1
# "merge" defines how and when pull requests are merged. If the section is
# missing, bulldozer will consider all pull requests and use default settings.
merge:
# "whitelist" defines the set of pull requests considered by bulldozer. If
# "trigger" defines the set of pull requests considered by bulldozer. If
# the section is missing, bulldozer considers all pull requests not excluded
# by the blacklist.
whitelist:
# by the ignore conditions.
trigger:
# Pull requests with any of these labels (case-insensitive) are added to
# the whitelist.
# the trigger.
labels: ["merge when ready"]

# Pull requests where the body or any comment contains any of these
# substrings are added to the whitelist.
# substrings are added to the trigger.
comment_substrings: ["==MERGE_WHEN_READY=="]

# Pull requests where any comment matches one of these exact strings are
# added to the whitelist.
# added to the trigger.
comments: ["Please merge this pull request!"]

# Pull requests where the body contains any of these substrings are added
# to the whitelist.
# to the trigger.
pr_body_substrings: ["==MERGE_WHEN_READY=="]

# Pull requests targeting any of these branches are added to the whitelist.
# Pull requests targeting any of these branches are added to the trigger.
branches: ["develop"]

# "blacklist" defines the set of pull request ignored by bulldozer. If the
# "ignore" defines the set of pull request ignored by bulldozer. If the
# section is missing, bulldozer considers all pull requests. It takes the
# same keys as the "whitelist" section.
blacklist:
# same keys as the "trigger" section.
ignore:
labels: ["do not merge"]
comment_substrings: ["==DO_NOT_MERGE=="]

Expand Down Expand Up @@ -148,23 +148,23 @@ merge:
# "update" defines how and when to update pull request branches. Unlike with
# merges, if this section is missing, bulldozer will not update any pull requests.
update:
# "whitelist" defines the set of pull requests that should be updated by
# bulldozer. It accepts the same keys as the whitelist in the "merge" block.
whitelist:
# "trigger" defines the set of pull requests that should be updated by
# bulldozer. It accepts the same keys as the trigger in the "merge" block.
trigger:
labels: ["WIP", "Update Me"]

# "blacklist" defines the set of pull requests that should not be updated by
# bulldozer. It accepts the same keys as the blacklist in the "merge" block.
blacklist:
# "ignore" defines the set of pull requests that should not be updated by
# bulldozer. It accepts the same keys as the ignore in the "merge" block.
ignore:
labels: ["Do Not Update"]
```
## FAQ
#### Can I specify both a `blacklist` and `whitelist`?
#### Can I specify both `ignore` and `trigger`?

Yes. If both `blacklist` and `whitelist` are specified, bulldozer will attempt to match
on both. In cases where both match, `blacklist` will take precedence.
Yes. If both `ignore` and `trigger` are specified, bulldozer will attempt to match
on both. In cases where both match, `ignore` will take precedence.

#### Can I specify the body of the commit when using the `squash` strategy?

Expand Down Expand Up @@ -203,13 +203,13 @@ It will be used only when your repo config file does not exist.
```yaml
options:
default_repository_config:
blacklist:
ignore:
labels: ["do not merge"] # or any other available config.
```

#### Bulldozer isn't merging my commit when it should, what could be happening?

Bulldozer will attempt to merge a branch whenever it passes the whitelist/blacklist
Bulldozer will attempt to merge a branch whenever it passes the trigger/ignore
criteria. GitHub may prevent it from merging a branch in certain conditions, some of
which are to be expected, and others that may be caused by mis-configuring Bulldozer.

Expand All @@ -219,7 +219,7 @@ which are to be expected, and others that may be caused by mis-configuring Bulld
repository settings
* Branch protection rules are preventing `bulldozer[bot]` from [pushing to the
branch][push restrictions]. Github apps can be added to the list of restricted
push users, so you can whitelist bulldozer specifically for your repo.
push users, so you can allow Bulldozer specifically for your repo.

[push restrictions]: https://help.github.com/articles/about-branch-restrictions/
[a workaround]: #can-bulldozer-work-with-push-restrictions-on-branches
Expand Down
26 changes: 20 additions & 6 deletions bulldozer/config_fetcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,20 @@ func (cf *ConfigFetcher) unmarshalConfig(bytes []byte) (*Config, error) {
return nil, errors.Wrapf(err, "failed to unmarshal configuration")
}

// Merge old signals configurations if they exist when the new values aren't present
if config.Merge.Blacklist.Enabled() && !config.Merge.Ignore.Enabled() {
config.Merge.Ignore = config.Merge.Blacklist
}
if config.Merge.Whitelist.Enabled() && !config.Merge.Trigger.Enabled() {
config.Merge.Trigger = config.Merge.Whitelist
}
if config.Update.Blacklist.Enabled() && !config.Update.Ignore.Enabled() {
config.Update.Ignore = config.Update.Blacklist
}
if config.Update.Whitelist.Enabled() && !config.Update.Trigger.Enabled() {
config.Update.Trigger = config.Update.Whitelist
}

if config.Version != 1 {
return nil, errors.Errorf("unexpected version '%d', expected 1", config.Version)
}
Expand All @@ -167,12 +181,12 @@ func (cf *ConfigFetcher) unmarshalConfigV0(bytes []byte) (*Config, error) {
config = Config{
Version: 1,
Update: UpdateConfig{
Whitelist: Signals{
Trigger: Signals{
Labels: []string{"update me", "update-me", "update_me"},
},
},
Merge: MergeConfig{
Whitelist: Signals{
Trigger: Signals{
Labels: []string{"merge when ready", "merge-when-ready", "merge_when_ready"},
},
DeleteAfterMerge: configv0.DeleteAfterMerge,
Expand All @@ -188,12 +202,12 @@ func (cf *ConfigFetcher) unmarshalConfigV0(bytes []byte) (*Config, error) {
config = Config{
Version: 1,
Update: UpdateConfig{
Whitelist: Signals{
Trigger: Signals{
Labels: []string{"update me", "update-me", "update_me"},
},
},
Merge: MergeConfig{
Blacklist: Signals{
Ignore: Signals{
Labels: []string{"wip", "do not merge", "do-not-merge", "do_not_merge"},
},
DeleteAfterMerge: configv0.DeleteAfterMerge,
Expand All @@ -209,12 +223,12 @@ func (cf *ConfigFetcher) unmarshalConfigV0(bytes []byte) (*Config, error) {
config = Config{
Version: 1,
Update: UpdateConfig{
Whitelist: Signals{
Trigger: Signals{
Labels: []string{"update me", "update-me", "update_me"},
},
},
Merge: MergeConfig{
Whitelist: Signals{
Trigger: Signals{
CommentSubstrings: []string{"==MERGE_WHEN_READY=="},
},
DeleteAfterMerge: configv0.DeleteAfterMerge,
Expand Down
160 changes: 160 additions & 0 deletions bulldozer/config_fetcher_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
// Copyright 2020 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 bulldozer

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestMarshalling(t *testing.T) {
t.Run("parseNewConfig", func(t *testing.T) {
cf := NewConfigFetcher("", []string{""}, nil)

config := `
version: 1
merge:
trigger:
labels: ["merge when ready"]
comment_substrings: ["==MERGE_WHEN_READY=="]
ignore:
labels: ["do not merge"]
comment_substrings: ["==DO_NOT_MERGE=="]
method: squash
options:
squash:
body: summarize_commits
delete_after_merge: true
update:
trigger:
labels: ["wip", "update me"]
ignore:
labels: ["do not update"]
`

actual, err := cf.unmarshalConfig([]byte(config))
require.Nil(t, err)
assert.Equal(t, Signals{
Labels: []string{"merge when ready"},
CommentSubstrings: []string{"==MERGE_WHEN_READY=="},
}, actual.Merge.Trigger)
assert.Equal(t, Signals{
Labels: []string{"do not merge"},
CommentSubstrings: []string{"==DO_NOT_MERGE=="},
}, actual.Merge.Ignore)
})

t.Run("parseExisting", func(t *testing.T) {
cf := NewConfigFetcher("", []string{""}, nil)

config := `
version: 1
merge:
whitelist:
labels: ["merge when ready"]
comment_substrings: ["==OLD_MERGE_WHEN_READY=="]
blacklist:
labels: ["do not merge"]
comment_substrings: ["==OLD_DO_NOT_MERGE=="]
method: squash
options:
squash:
body: summarize_commits
delete_after_merge: true
update:
whitelist:
labels: ["wip", "update me"]
blacklist:
labels: ["do not update"]
`

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

assert.Equal(t, Signals{
Labels: []string{"merge when ready"},
CommentSubstrings: []string{"==OLD_MERGE_WHEN_READY=="},
}, actual.Merge.Trigger)
assert.Equal(t, Signals{
Labels: []string{"do not merge"},
CommentSubstrings: []string{"==OLD_DO_NOT_MERGE=="},
}, actual.Merge.Ignore)

assert.Equal(t, Signals{
Labels: []string{"wip", "update me"},
}, actual.Update.Trigger)
assert.Equal(t, Signals{
Labels: []string{"do not update"},
}, actual.Update.Ignore)
})

t.Run("ignoresOldConfig", func(t *testing.T) {
cf := NewConfigFetcher("", []string{""}, nil)

config := `
version: 1
merge:
trigger:
labels: ["mwr"]
ignore:
labels: ["new dnm"]
whitelist:
labels: ["merge when ready"]
comment_substrings: ["==OLD_MERGE_WHEN_READY=="]
blacklist:
labels: ["do not merge"]
comment_substrings: ["==OLD_DO_NOT_MERGE=="]
method: squash
options:
squash:
body: summarize_commits
delete_after_merge: true
update:
trigger:
labels: ["new wip"]
ignore:
labels: ["new dnu"]
whitelist:
labels: ["wip", "update me"]
blacklist:
labels: ["do not update"]
`

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

assert.Equal(t, Signals{
Labels: []string{"mwr"},
}, actual.Merge.Trigger)
assert.Equal(t, Signals{
Labels: []string{"new dnm"},
}, actual.Merge.Ignore)

assert.Equal(t, Signals{
Labels: []string{"new wip"},
}, actual.Update.Trigger)
assert.Equal(t, Signals{
Labels: []string{"new dnu"},
}, actual.Update.Ignore)
})
}
12 changes: 10 additions & 2 deletions bulldozer/config_v1.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,12 @@ const (
)

type MergeConfig struct {
Whitelist Signals `yaml:"whitelist"`
Trigger Signals `yaml:"trigger"`
Ignore Signals `yaml:"ignore"`

// Blacklist and Whitelist are legacy options that will be disabled in a future v2 format
Blacklist Signals `yaml:"blacklist"`
Whitelist Signals `yaml:"whitelist"`

DeleteAfterMerge bool `yaml:"delete_after_merge"`

Expand All @@ -60,8 +64,12 @@ type SquashOptions struct {
}

type UpdateConfig struct {
Whitelist Signals `yaml:"whitelist"`
Trigger Signals `yaml:"trigger"`
Ignore Signals `yaml:"ignore"`

// Blacklist and Whitelist are legacy options that will be disabled in a future v2 format
Blacklist Signals `yaml:"blacklist"`
Whitelist Signals `yaml:"whitelist"`
}

type Config struct {
Expand Down
Loading

0 comments on commit 88d5aed

Please sign in to comment.