diff --git a/prow/apis/prowjobs/v1/types.go b/prow/apis/prowjobs/v1/types.go index 683f3b55e872..4df22bae90ed 100644 --- a/prow/apis/prowjobs/v1/types.go +++ b/prow/apis/prowjobs/v1/types.go @@ -292,6 +292,35 @@ type SlackReporterConfig struct { ReportTemplate string `json:"report_template,omitempty"` } +func (src *SlackReporterConfig) ApplyDefault(def *SlackReporterConfig) *SlackReporterConfig { + if src == nil && def == nil { + return nil + } + var merged SlackReporterConfig + if src != nil { + merged = *src.DeepCopy() + } else { + merged = *def.DeepCopy() + } + if src == nil || def == nil { + return &merged + } + + if merged.Channel == "" { + merged.Channel = def.Channel + } + if merged.Host == "" { + merged.Host = def.Host + } + if merged.JobStatesToReport == nil { + merged.JobStatesToReport = def.JobStatesToReport + } + if merged.ReportTemplate == "" { + merged.ReportTemplate = def.ReportTemplate + } + return &merged +} + // Duration is a wrapper around time.Duration that parses times in either // 'integer number of nanoseconds' or 'duration string' formats and serializes // to 'duration string' format. diff --git a/prow/apis/prowjobs/v1/types_test.go b/prow/apis/prowjobs/v1/types_test.go index b03677bfc5f9..ce3c88571eeb 100644 --- a/prow/apis/prowjobs/v1/types_test.go +++ b/prow/apis/prowjobs/v1/types_test.go @@ -301,6 +301,36 @@ func TestApplyDefaultsAppliesDefaultsForAllFields(t *testing.T) { } } +func TestSlackConfigApplyDefaultsAppliesDefaultsForAllFields(t *testing.T) { + t.Parallel() + seed := time.Now().UnixNano() + // Print the seed so failures can easily be reproduced + t.Logf("Seed: %d", seed) + fuzzer := fuzz.NewWithSeed(seed) + for i := 0; i < 100; i++ { + t.Run(strconv.Itoa(i), func(t *testing.T) { + def := &SlackReporterConfig{} + fuzzer.Fuzz(def) + + // Each of those three has its own DeepCopy and in case it is nil, + // we just call that and return. In order to make this test verify + // that copying of their fields also works, we have to set them to + // something non-nil. + toDefault := &SlackReporterConfig{ + Host: "", + Channel: "", + JobStatesToReport: nil, + ReportTemplate: "", + } + defaulted := toDefault.ApplyDefault(def) + + if diff := cmp.Diff(def, defaulted); diff != "" { + t.Errorf("defaulted decoration config didn't get all fields defaulted: %s", diff) + } + }) + } +} + func TestRefsToString(t *testing.T) { var tests = []struct { name string diff --git a/prow/config/config.go b/prow/config/config.go index a2011be1ca85..50202606516a 100644 --- a/prow/config/config.go +++ b/prow/config/config.go @@ -1042,11 +1042,8 @@ type ManagedWebhooks struct { // SlackReporter represents the config for the Slack reporter. The channel can be overridden // on the job via the .reporter_config.slack.channel property type SlackReporter struct { - JobTypesToReport []prowapi.ProwJobType `json:"job_types_to_report,omitempty"` - JobStatesToReport []prowapi.ProwJobState `json:"job_states_to_report,omitempty"` - Host string `json:"host,omitempty"` - Channel string `json:"channel"` - ReportTemplate string `json:"report_template"` + JobTypesToReport []prowapi.ProwJobType `json:"job_types_to_report,omitempty"` + prowapi.SlackReporterConfig `json:",inline"` } // SlackReporterConfigs represents the config for the Slack reporter(s). diff --git a/prow/config/config_test.go b/prow/config/config_test.go index ed6013e5ada0..c91738c79b9c 100644 --- a/prow/config/config_test.go +++ b/prow/config/config_test.go @@ -3330,7 +3330,9 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "*": { - Channel: "my-channel", + SlackReporterConfig: prowjobv1.SlackReporterConfig{ + Channel: "my-channel", + }, }, } return Config{ @@ -3346,7 +3348,9 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "istio/proxy": { - Channel: "my-channel", + SlackReporterConfig: prowjobv1.SlackReporterConfig{ + Channel: "my-channel", + }, }, } return Config{ @@ -3362,7 +3366,9 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "proxy": { - Channel: "my-channel", + SlackReporterConfig: prowjobv1.SlackReporterConfig{ + Channel: "my-channel", + }, }, } return Config{ @@ -3406,8 +3412,10 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "*": { - Channel: "my-channel", - ReportTemplate: "{{ if .Spec.Name}}", + SlackReporterConfig: prowjobv1.SlackReporterConfig{ + Channel: "my-channel", + ReportTemplate: "{{ if .Spec.Name}}", + }, }, } return Config{ @@ -3423,8 +3431,10 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "*": { - Channel: "my-channel", - ReportTemplate: "{{ .Undef}}", + SlackReporterConfig: prowjobv1.SlackReporterConfig{ + Channel: "my-channel", + ReportTemplate: "{{ .Undef}}", + }, }, } return Config{ diff --git a/prow/crier/reporters/slack/reporter.go b/prow/crier/reporters/slack/reporter.go index b725d5714941..b835fb42ab7e 100644 --- a/prow/crier/reporters/slack/reporter.go +++ b/prow/crier/reporters/slack/reporter.go @@ -19,6 +19,7 @@ package slack import ( "bytes" "context" + "errors" "fmt" "text/template" @@ -46,40 +47,25 @@ type slackReporter struct { dryRun bool } -func (sr *slackReporter) getConfig(pj *v1.ProwJob) config.SlackReporter { - refs := pj.Spec.Refs - if refs == nil && len(pj.Spec.ExtraRefs) > 0 { - refs = &pj.Spec.ExtraRefs[0] - } - return sr.config(refs) -} - -func jobConfig(pj *v1.ProwJob) *v1.SlackReporterConfig { - if pj.Spec.ReporterConfig != nil { - return pj.Spec.ReporterConfig.Slack - } - return nil -} - -func channel(prowCfg config.SlackReporter, jobCfg *v1.SlackReporterConfig) (string, string) { - host, channel := prowCfg.Host, prowCfg.Channel - if jobCfg != nil && jobCfg.Host != "" { - host = jobCfg.Host - } - if jobCfg != nil && jobCfg.Channel != "" { - channel = jobCfg.Channel - } - if len(host) == 0 { +func hostAndChannel(cfg *v1.SlackReporterConfig) (string, string) { + host, channel := cfg.Host, cfg.Channel + if host == "" { host = DefaultHostName } return host, channel } -func reportTemplate(prowCfg config.SlackReporter, jobCfg *v1.SlackReporterConfig) string { - if jobCfg != nil && jobCfg.ReportTemplate != "" { - return jobCfg.ReportTemplate +func (sr *slackReporter) getConfig(pj *v1.ProwJob) (*config.SlackReporter, *v1.SlackReporterConfig) { + refs := pj.Spec.Refs + if refs == nil && len(pj.Spec.ExtraRefs) > 0 { + refs = &pj.Spec.ExtraRefs[0] } - return prowCfg.ReportTemplate + globalConfig := sr.config(refs) + var jobSlackConfig *v1.SlackReporterConfig + if pj.Spec.ReporterConfig != nil && pj.Spec.ReporterConfig.Slack != nil { + jobSlackConfig = pj.Spec.ReporterConfig.Slack + } + return &globalConfig, jobSlackConfig } func (sr *slackReporter) Report(_ context.Context, log *logrus.Entry, pj *v1.ProwJob) ([]*v1.ProwJob, *reconcile.Result, error) { @@ -87,16 +73,21 @@ func (sr *slackReporter) Report(_ context.Context, log *logrus.Entry, pj *v1.Pro } func (sr *slackReporter) report(log *logrus.Entry, pj *v1.ProwJob) error { - prowCfg := sr.getConfig(pj) - jobCfg := jobConfig(pj) - templateStr := reportTemplate(prowCfg, jobCfg) - host, channel := channel(prowCfg, jobCfg) + globalSlackConfig, jobSlackConfig := sr.getConfig(pj) + if globalSlackConfig != nil { + jobSlackConfig = jobSlackConfig.ApplyDefault(&globalSlackConfig.SlackReporterConfig) + } + if jobSlackConfig == nil { + return errors.New("resolved slack config is empty") // Shouldn't happen at all, just in case + } + host, channel := hostAndChannel(jobSlackConfig) + client, ok := sr.clients[host] if !ok { return fmt.Errorf("host '%s' not supported", host) } b := &bytes.Buffer{} - tmpl, err := template.New("").Parse(templateStr) + tmpl, err := template.New("").Parse(jobSlackConfig.ReportTemplate) if err != nil { log.WithError(err).Error("failed to parse template") return fmt.Errorf("failed to parse template: %v", err) @@ -121,23 +112,22 @@ func (sr *slackReporter) GetName() string { } func (sr *slackReporter) ShouldReport(_ context.Context, logger *logrus.Entry, pj *v1.ProwJob) bool { - jobCfg := jobConfig(pj) - prowCfg := sr.getConfig(pj) - - // The job needs to be reported, if its type has a match with the - // JobTypesToReport in the Prow config. - typeShouldReport := false - for _, typeToReport := range prowCfg.JobTypesToReport { - if typeToReport == pj.Spec.Type { - typeShouldReport = true - break + globalSlackConfig, jobSlackConfig := sr.getConfig(pj) + + var typeShouldReport bool + if globalSlackConfig.JobTypesToReport != nil { + for _, tp := range globalSlackConfig.JobTypesToReport { + if tp == pj.Spec.Type { + typeShouldReport = true + break + } } } // If a user specifically put a channel on their job, they want // it to be reported regardless of the job types setting. - jobShouldReport := false - if jobCfg != nil && jobCfg.Channel != "" { + var jobShouldReport bool + if jobSlackConfig != nil && jobSlackConfig.Channel != "" { jobShouldReport = true } @@ -145,15 +135,13 @@ func (sr *slackReporter) ShouldReport(_ context.Context, logger *logrus.Entry, p // JobStatesToReport config. // Note the JobStatesToReport configured in the Prow job can overwrite the // Prow config. - jobStatesToReport := prowCfg.JobStatesToReport - if jobCfg != nil && len(jobCfg.JobStatesToReport) != 0 { - jobStatesToReport = jobCfg.JobStatesToReport - } - stateShouldReport := false - for _, stateToReport := range jobStatesToReport { - if pj.Status.State == stateToReport { - stateShouldReport = true - break + var stateShouldReport bool + if merged := jobSlackConfig.ApplyDefault(&globalSlackConfig.SlackReporterConfig); merged != nil && merged.JobStatesToReport != nil { + for _, stateToReport := range merged.JobStatesToReport { + if pj.Status.State == stateToReport { + stateShouldReport = true + break + } } } diff --git a/prow/crier/reporters/slack/reporter_test.go b/prow/crier/reporters/slack/reporter_test.go index ac9cfda0f40c..1abe7f08fa45 100644 --- a/prow/crier/reporters/slack/reporter_test.go +++ b/prow/crier/reporters/slack/reporter_test.go @@ -36,8 +36,10 @@ func TestShouldReport(t *testing.T) { { name: "Presubmit Job should report", config: config.SlackReporter{ - JobTypesToReport: []v1.ProwJobType{v1.PresubmitJob}, - JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + JobTypesToReport: []v1.ProwJobType{v1.PresubmitJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -50,10 +52,12 @@ func TestShouldReport(t *testing.T) { expected: true, }, { - name: "Presubmit Job should not report", + name: "Wrong job type should not report", config: config.SlackReporter{ - JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, - JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -68,8 +72,10 @@ func TestShouldReport(t *testing.T) { { name: "Successful Job should report", config: config.SlackReporter{ - JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, - JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -81,11 +87,55 @@ func TestShouldReport(t *testing.T) { }, expected: true, }, + { + name: "Empty job config settings negate global", + config: config.SlackReporter{ + JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + }, + }, + pj: &v1.ProwJob{ + Spec: v1.ProwJobSpec{ + Type: v1.PostsubmitJob, + ReporterConfig: &v1.ReporterConfig{ + Slack: &v1.SlackReporterConfig{JobStatesToReport: []v1.ProwJobState{}}, + }, + }, + Status: v1.ProwJobStatus{ + State: v1.SuccessState, + }, + }, + expected: false, + }, + { + name: "Nil job config settings does not negate global", + config: config.SlackReporter{ + JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + }, + }, + pj: &v1.ProwJob{ + Spec: v1.ProwJobSpec{ + Type: v1.PostsubmitJob, + ReporterConfig: &v1.ReporterConfig{ + Slack: &v1.SlackReporterConfig{JobStatesToReport: nil}, + }, + }, + Status: v1.ProwJobStatus{ + State: v1.SuccessState, + }, + }, + expected: true, + }, { name: "Successful Job should not report", config: config.SlackReporter{ - JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, - JobStatesToReport: []v1.ProwJobState{v1.PendingState}, + JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.PendingState}, + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -100,8 +150,10 @@ func TestShouldReport(t *testing.T) { { name: "Job with channel config should ignore the JobTypesToReport config", config: config.SlackReporter{ - JobTypesToReport: []v1.ProwJobType{}, - JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + JobTypesToReport: []v1.ProwJobType{}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -119,8 +171,10 @@ func TestShouldReport(t *testing.T) { { name: "JobStatesToReport in Job config should override the one in Prow config", config: config.SlackReporter{ - JobTypesToReport: []v1.ProwJobType{}, - JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + JobTypesToReport: []v1.ProwJobType{}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -141,8 +195,10 @@ func TestShouldReport(t *testing.T) { { name: "Job with channel config but does not have matched state in Prow config should not report", config: config.SlackReporter{ - JobTypesToReport: []v1.ProwJobType{}, - JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + JobTypesToReport: []v1.ProwJobType{}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -160,8 +216,10 @@ func TestShouldReport(t *testing.T) { { name: "Job with channel and state config where the state does not match, should not report", config: config.SlackReporter{ - JobTypesToReport: []v1.ProwJobType{}, - JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + JobTypesToReport: []v1.ProwJobType{}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -255,8 +313,10 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - Host: "global-default-host", - Channel: "global-default", + SlackReporterConfig: v1.SlackReporterConfig{ + Host: "global-default-host", + Channel: "global-default", + }, }, } return config.Config{ @@ -274,11 +334,15 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - Channel: "global-default", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "global-default", + }, }, "istio/proxy": { - Host: "global-default-host", - Channel: "org-repo-config", + SlackReporterConfig: v1.SlackReporterConfig{ + Host: "global-default-host", + Channel: "org-repo-config", + }, }, } return config.Config{ @@ -302,10 +366,14 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - Channel: "global-default", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "global-default", + }, }, "istio": { - Channel: "org-config", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-config", + }, }, } return config.Config{ @@ -329,13 +397,19 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - Channel: "global-default", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "global-default", + }, }, "istio": { - Channel: "org-config", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-config", + }, }, "istio/proxy": { - Channel: "org-repo-config", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-repo-config", + }, }, } return config.Config{ @@ -359,13 +433,19 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - Channel: "global-default", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "global-default", + }, }, "istio": { - Channel: "org-config", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-config", + }, }, "istio/proxy": { - Channel: "org-repo-config", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-repo-config", + }, }, } return config.Config{ @@ -391,10 +471,14 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "istio": { - Channel: "org-config", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-config", + }, }, "istio/proxy": { - Channel: "org-repo-config", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-repo-config", + }, }, } return config.Config{ @@ -418,7 +502,9 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "istio/proxy": { - Channel: "org-repo-config", + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-repo-config", + }, }, } return config.Config{ @@ -449,9 +535,9 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: cfgGetter, } - prowSlackCfg := sr.getConfig(tc.pj) - jobSlackCfg := jobConfig(tc.pj) - gotHost, gotChannel := channel(prowSlackCfg, jobSlackCfg) + prowSlackCfg, jobSlackCfg := sr.getConfig(tc.pj) + jobSlackCfg = jobSlackCfg.ApplyDefault(&prowSlackCfg.SlackReporterConfig) + gotHost, gotChannel := hostAndChannel(jobSlackCfg) if gotHost != tc.wantHost { t.Fatalf("Expected host: %q, got: %q", tc.wantHost, gotHost) } @@ -476,8 +562,10 @@ func TestShouldReportDefaultsToExtraRefs(t *testing.T) { config: func(r *v1.Refs) config.SlackReporter { if r.Org == "org" { return config.SlackReporter{ - JobTypesToReport: []v1.ProwJobType{v1.PeriodicJob}, - JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + JobTypesToReport: []v1.ProwJobType{v1.PeriodicJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + }, } } return config.SlackReporter{} @@ -518,10 +606,12 @@ func TestReportDefaultsToExtraRefs(t *testing.T) { config: func(r *v1.Refs) config.SlackReporter { if r.Org == "org" { return config.SlackReporter{ - JobTypesToReport: []v1.ProwJobType{v1.PeriodicJob}, - JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, - Channel: "emercengy", - ReportTemplate: "there you go", + JobTypesToReport: []v1.ProwJobType{v1.PeriodicJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + Channel: "emercengy", + ReportTemplate: "there you go", + }, } } return config.SlackReporter{}