Skip to content

Commit

Permalink
Use pointer for global Blunderbuss config
Browse files Browse the repository at this point in the history
Using a pointer lets sharding determine whether a config had been
provided so they can be properly compared. If one is not provided, this
also instantiates one in `setDefaults()`, which is called following the
sharding logic.

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>
  • Loading branch information
dhaiducek committed May 3, 2023
1 parent 1e3d1af commit ec92a0f
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 26 deletions.
2 changes: 1 addition & 1 deletion prow/cmd/deck/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -831,7 +831,7 @@ func TestHandlePluginConfig(t *testing.T) {
}},
},
Blunderbuss: plugins.Blunderbuss{
BlunderbussConfig: plugins.BlunderbussConfig{
BlunderbussConfig: &plugins.BlunderbussConfig{
ExcludeApprovers: true,
}},
}
Expand Down
2 changes: 1 addition & 1 deletion prow/plugins/blunderbuss/blunderbuss.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func helpProvider(config *plugins.Configuration, _ []config.OrgRepo) (*pluginhel
two := 2
yamlSnippet, err := plugins.CommentMap.GenYaml(&plugins.Configuration{
Blunderbuss: plugins.Blunderbuss{
BlunderbussConfig: plugins.BlunderbussConfig{
BlunderbussConfig: &plugins.BlunderbussConfig{
ReviewerCount: &two,
MaxReviewerCount: 3,
ExcludeApprovers: true,
Expand Down
23 changes: 16 additions & 7 deletions prow/plugins/blunderbuss/blunderbuss_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,7 @@ func TestHandlePullRequestShardedConfig(t *testing.T) {
fghc := newFakeGitHubClient(&pr, tc.filesChanged)
c := &plugins.Configuration{
Blunderbuss: plugins.Blunderbuss{
BlunderbussConfig: plugins.BlunderbussConfig{
BlunderbussConfig: &plugins.BlunderbussConfig{
ReviewerCount: &tc.reviewerCount,
MaxReviewerCount: 0,
ExcludeApprovers: false,
Expand Down Expand Up @@ -897,7 +897,7 @@ func TestHandleGenericCommentShardedConfig(t *testing.T) {

config := &plugins.Configuration{
Blunderbuss: plugins.Blunderbuss{
BlunderbussConfig: plugins.BlunderbussConfig{
BlunderbussConfig: &plugins.BlunderbussConfig{
IgnoreAuthors: []string{"bob"},
ReviewerCount: &defaultReviewerCount,
UseStatusAvailability: false,
Expand All @@ -922,15 +922,21 @@ func TestHandleGenericCommentShardedConfig(t *testing.T) {

func TestHandleGenericCommentEvent(t *testing.T) {
pc := plugins.Agent{
PluginConfig: &plugins.Configuration{},
PluginConfig: &plugins.Configuration{
Blunderbuss: plugins.Blunderbuss{
BlunderbussConfig: &plugins.BlunderbussConfig{},
}},
}
ce := github.GenericCommentEvent{}
handleGenericCommentEvent(pc, ce)
}

func TestHandlePullRequestEvent(t *testing.T) {
pc := plugins.Agent{
PluginConfig: &plugins.Configuration{},
PluginConfig: &plugins.Configuration{
Blunderbuss: plugins.Blunderbuss{
BlunderbussConfig: &plugins.BlunderbussConfig{},
}},
}
pre := github.PullRequestEvent{}
handlePullRequestEvent(pc, pre)
Expand All @@ -949,16 +955,19 @@ func TestHelpProvider(t *testing.T) {
configInfoIncludes []string
}{
{
name: "Empty config",
config: &plugins.Configuration{},
name: "Empty config",
config: &plugins.Configuration{
Blunderbuss: plugins.Blunderbuss{
BlunderbussConfig: &plugins.BlunderbussConfig{},
}},
enabledRepos: enabledRepos,
configInfoIncludes: []string{configString(0)},
},
{
name: "ReviewerCount specified",
config: &plugins.Configuration{
Blunderbuss: plugins.Blunderbuss{
BlunderbussConfig: plugins.BlunderbussConfig{
BlunderbussConfig: &plugins.BlunderbussConfig{
ReviewerCount: &[]int{2}[0],
}},
},
Expand Down
21 changes: 15 additions & 6 deletions prow/plugins/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ type BlunderbussOrgConfig struct {

// Blunderbuss defines overall configuration for the blunderbuss plugin.
type Blunderbuss struct {
BlunderbussConfig `json:",inline,omitempty"`
*BlunderbussConfig `json:",inline,omitempty"`
// Orgs allows sharding for organization-specific Blunderbuss settings, provided under keys with
// the name of the organization. For repository-specific settings, provide
// organization/repository keys under orgs[].repos.
Expand Down Expand Up @@ -891,7 +891,7 @@ func (c *Configuration) BlunderbussFor(org, repo string) BlunderbussConfig {
}

// Return base config
return c.Blunderbuss.BlunderbussConfig
return *c.Blunderbuss.BlunderbussConfig
}

// LgtmFor finds the Lgtm for a repo, if one exists
Expand Down Expand Up @@ -1068,6 +1068,10 @@ func (c *Configuration) setDefaults() {
c.ExternalPlugins[repo][i].Endpoint = fmt.Sprintf("http://%s", p.Name)
}
}
// Instantiate a global Blunderbuss config if it wasn't set in the config
if c.Blunderbuss.BlunderbussConfig == nil {
c.Blunderbuss.BlunderbussConfig = &BlunderbussConfig{}
}
if c.Blunderbuss.ReviewerCount == nil {
c.Blunderbuss.ReviewerCount = new(int)
*c.Blunderbuss.ReviewerCount = defaultBlunderbussReviewerCount
Expand Down Expand Up @@ -2115,9 +2119,14 @@ func (b *Blunderbuss) mergeFrom(other *Blunderbuss) error {

var errs []error

// Add error when both configs declare different global defaults
if !reflect.DeepEqual(b.BlunderbussConfig, other.BlunderbussConfig) {
errs = append(errs, fmt.Errorf("global configurations for blunderbuss do not match"))
// Handle global configs
if other.BlunderbussConfig != nil {
if b.BlunderbussConfig == nil {
b.BlunderbussConfig = other.BlunderbussConfig
} else if !reflect.DeepEqual(b.BlunderbussConfig, other.BlunderbussConfig) {
// Add error when both configs declare different global defaults
errs = append(errs, fmt.Errorf("global configurations for blunderbuss do not match"))
}
}

// Initialize the Orgs map if it's empty to accept incoming Orgs configs
Expand Down Expand Up @@ -2180,7 +2189,7 @@ func (c *Configuration) HasConfigFor() (global bool, orgs sets.String, repos set
ExternalPlugins: c.ExternalPlugins, Label: Label{RestrictedLabels: c.Label.RestrictedLabels},
Lgtm: c.Lgtm, Plugins: c.Plugins, Triggers: c.Triggers, Welcome: c.Welcome})

if !equals || c.Bugzilla.Default != nil {
if !equals || c.Bugzilla.Default != nil || c.Blunderbuss.BlunderbussConfig != nil {
global = true
}
orgs = sets.String{}
Expand Down
38 changes: 28 additions & 10 deletions prow/plugins/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1731,34 +1731,52 @@ func TestBlunderbussMergeFrom(t *testing.T) {
Orgs: map[string]BlunderbussOrgConfig{"org": {
Repos: map[string]BlunderbussRepoConfig{"org/repo-1": {}}}}},
to: &Blunderbuss{
BlunderbussConfig: BlunderbussConfig{}},
BlunderbussConfig: &BlunderbussConfig{}},
expected: &Blunderbuss{
BlunderbussConfig: BlunderbussConfig{},
BlunderbussConfig: &BlunderbussConfig{},
Orgs: map[string]BlunderbussOrgConfig{"org": {
Repos: map[string]BlunderbussRepoConfig{"org/repo-1": {}}}}},
},
{
name: "Merging org/repo config with global config succeeds",
from: &Blunderbuss{
BlunderbussConfig: BlunderbussConfig{}},
BlunderbussConfig: &BlunderbussConfig{}},
to: &Blunderbuss{
Orgs: map[string]BlunderbussOrgConfig{"org": {
Repos: map[string]BlunderbussRepoConfig{"org/repo-1": {}}}}},
expected: &Blunderbuss{
BlunderbussConfig: BlunderbussConfig{},
BlunderbussConfig: &BlunderbussConfig{},
Orgs: map[string]BlunderbussOrgConfig{"org": {
Repos: map[string]BlunderbussRepoConfig{"org/repo-1": {}}}}},
},
{
name: "Merging identical global configs succeeds",
from: &Blunderbuss{BlunderbussConfig: BlunderbussConfig{}},
to: &Blunderbuss{BlunderbussConfig: BlunderbussConfig{}},
expected: &Blunderbuss{BlunderbussConfig: BlunderbussConfig{}},
from: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{}},
to: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{}},
expected: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{}},
},
{
name: "Merging from nil config succeeds",
from: nil,
to: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{}},
expected: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{}},
},
{
name: "Merging to nil global config succeeds",
from: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{}},
to: &Blunderbuss{BlunderbussConfig: nil},
expected: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{}},
},
{
name: "Merging from config with nil global config succeeds",
from: &Blunderbuss{BlunderbussConfig: nil},
to: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{}},
expected: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{}},
},
{
name: "Merging differing global configs fails",
from: &Blunderbuss{BlunderbussConfig: BlunderbussConfig{MaxReviewerCount: 1}},
to: &Blunderbuss{BlunderbussConfig: BlunderbussConfig{MaxReviewerCount: 2}},
from: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{MaxReviewerCount: 1}},
to: &Blunderbuss{BlunderbussConfig: &BlunderbussConfig{MaxReviewerCount: 2}},
expectedErrMsg: "global configurations for blunderbuss do not match",
},
{
Expand Down Expand Up @@ -2163,7 +2181,7 @@ func TestHasConfigFor(t *testing.T) {
fuzzedConfig.Triggers = nil
fuzzedConfig.Welcome = nil
fuzzedConfig.ExternalPlugins = nil
fuzzedConfig.Blunderbuss = Blunderbuss{BlunderbussConfig: BlunderbussConfig{}}
fuzzedConfig.Blunderbuss = Blunderbuss{}
return fuzzedConfig, !reflect.DeepEqual(fuzzedConfig, &Configuration{}), nil, nil
},
},
Expand Down
2 changes: 1 addition & 1 deletion prow/plugins/plugins_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ func TestLoad(t *testing.T) {
cfg := &Configuration{
Owners: Owners{LabelsDenyList: []string{"approved", "lgtm"}},
Blunderbuss: Blunderbuss{
BlunderbussConfig: BlunderbussConfig{ReviewerCount: func() *int { i := 2; return &i }()}},
BlunderbussConfig: &BlunderbussConfig{ReviewerCount: func() *int { i := 2; return &i }()}},
CherryPickUnapproved: CherryPickUnapproved{
BranchRegexp: "^release-.*$",
BranchRe: regexp.MustCompile("^release-.*$"),
Expand Down

0 comments on commit ec92a0f

Please sign in to comment.