Crier slack reporter: use pointers for merging configs

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.

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

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).

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{

prow/crier/reporters/slack/reporter.go

@@ -19,6 +19,7 @@ package slack
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"text/template"
 
@@ -46,57 +47,47 @@ 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) {
 	return []*v1.ProwJob{pj}, nil, sr.report(log, pj)
 }
 
 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,39 +112,36 @@ 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
 	}
 
 	// The job should only be reported if its state has a match with the
 	// 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
+			}
 		}
 	}