From fd3a3613a522728776fa9806bba032e86f88fd9f Mon Sep 17 00:00:00 2001 From: Chao Dai Date: Thu, 24 Jun 2021 08:07:28 -0700 Subject: [PATCH 1/2] Crier slack reporter: use pointers to better merging --- prow/apis/prowjobs/v1/types.go | 37 +++- .../apis/prowjobs/v1/zz_generated.deepcopy.go | 23 ++- prow/config/config.go | 16 +- prow/config/config_test.go | 30 ++- prow/config/prow-config-documented.yaml | 12 +- prow/crier/reporters/slack/reporter.go | 96 +++++----- prow/crier/reporters/slack/reporter_test.go | 181 +++++++++++++----- prow/pjutil/pjutil_test.go | 10 +- 8 files changed, 269 insertions(+), 136 deletions(-) diff --git a/prow/apis/prowjobs/v1/types.go b/prow/apis/prowjobs/v1/types.go index 683f3b55e872..91b6f3c89f1a 100644 --- a/prow/apis/prowjobs/v1/types.go +++ b/prow/apis/prowjobs/v1/types.go @@ -286,10 +286,39 @@ type ReporterConfig struct { } type SlackReporterConfig struct { - Host string `json:"host,omitempty"` - Channel string `json:"channel,omitempty"` - JobStatesToReport []ProwJobState `json:"job_states_to_report,omitempty"` - ReportTemplate string `json:"report_template,omitempty"` + Host *string `json:"host,omitempty"` + Channel *string `json:"channel,omitempty"` + JobStatesToReport *[]ProwJobState `json:"job_states_to_report,omitempty"` + 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 == nil { + merged.Channel = def.Channel + } + if merged.Host == nil { + merged.Host = def.Host + } + if merged.JobStatesToReport == nil { + merged.JobStatesToReport = def.JobStatesToReport + } + if merged.ReportTemplate == nil { + merged.ReportTemplate = def.ReportTemplate + } + return &merged } // Duration is a wrapper around time.Duration that parses times in either diff --git a/prow/apis/prowjobs/v1/zz_generated.deepcopy.go b/prow/apis/prowjobs/v1/zz_generated.deepcopy.go index b9b4d2eb386c..77cf060d723d 100644 --- a/prow/apis/prowjobs/v1/zz_generated.deepcopy.go +++ b/prow/apis/prowjobs/v1/zz_generated.deepcopy.go @@ -547,10 +547,29 @@ func (in *Resources) DeepCopy() *Resources { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SlackReporterConfig) DeepCopyInto(out *SlackReporterConfig) { *out = *in + if in.Host != nil { + in, out := &in.Host, &out.Host + *out = new(string) + **out = **in + } + if in.Channel != nil { + in, out := &in.Channel, &out.Channel + *out = new(string) + **out = **in + } if in.JobStatesToReport != nil { in, out := &in.JobStatesToReport, &out.JobStatesToReport - *out = make([]ProwJobState, len(*in)) - copy(*out, *in) + *out = new([]ProwJobState) + if **in != nil { + in, out := *in, *out + *out = make([]ProwJobState, len(*in)) + copy(*out, *in) + } + } + if in.ReportTemplate != nil { + in, out := &in.ReportTemplate, &out.ReportTemplate + *out = new(string) + **out = **in } return } diff --git a/prow/config/config.go b/prow/config/config.go index a2011be1ca85..a0b7f019956e 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 } // SlackReporterConfigs represents the config for the Slack reporter(s). @@ -1071,16 +1068,17 @@ func (cfg SlackReporterConfigs) GetSlackReporter(refs *prowapi.Refs) SlackReport func (cfg *SlackReporter) DefaultAndValidate() error { // Default ReportTemplate - if cfg.ReportTemplate == "" { - cfg.ReportTemplate = `Job {{.Spec.Job}} of type {{.Spec.Type}} ended with state {{.Status.State}}. <{{.Status.URL}}|View logs>` + if cfg.ReportTemplate == nil { + defaultTemplate := `Job {{.Spec.Job}} of type {{.Spec.Type}} ended with state {{.Status.State}}. <{{.Status.URL}}|View logs>` + cfg.ReportTemplate = &defaultTemplate } - if cfg.Channel == "" { + if cfg.Channel == nil || *cfg.Channel == "" { return errors.New("channel must be set") } // Validate ReportTemplate - tmpl, err := template.New("").Parse(cfg.ReportTemplate) + tmpl, err := template.New("").Parse(*cfg.ReportTemplate) if err != nil { return fmt.Errorf("failed to parse template: %v", err) } diff --git a/prow/config/config_test.go b/prow/config/config_test.go index ed6013e5ada0..23c61299af1f 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: prowapi.SlackReporterConfig{ + Channel: pStr("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: prowapi.SlackReporterConfig{ + Channel: pStr("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: prowapi.SlackReporterConfig{ + Channel: pStr("my-channel"), + }, }, } return Config{ @@ -3378,7 +3384,7 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "*": { - JobTypesToReport: []prowapi.ProwJobType{"presubmit"}, + JobTypesToReport: &[]prowapi.ProwJobType{"presubmit"}, }, } 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: prowapi.SlackReporterConfig{ + Channel: pStr("my-channel"), + ReportTemplate: pStr("{{ 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: prowapi.SlackReporterConfig{ + Channel: pStr("my-channel"), + ReportTemplate: pStr("{{ .Undef}}"), + }, }, } return Config{ @@ -3445,10 +3455,10 @@ func TestSlackReporterValidation(t *testing.T) { } if tc.successExpected { for _, config := range cfg.SlackReporterConfigs { - if config.ReportTemplate == "" { + if *config.ReportTemplate == "" { t.Errorf("expected default ReportTemplate to be set") } - if config.Channel == "" { + if *config.Channel == "" { t.Errorf("expected Channel to be required") } } diff --git a/prow/config/prow-config-documented.yaml b/prow/config/prow-config-documented.yaml index 49575bd3e3d4..1593b87e8137 100644 --- a/prow/config/prow-config-documented.yaml +++ b/prow/config/prow-config-documented.yaml @@ -947,13 +947,11 @@ sinker: terminated_pod_ttl: 0s slack_reporter_configs: "": - channel: ' ' - host: ' ' - job_states_to_report: - - "" - job_types_to_report: - - "" - report_template: ' ' + channel: "" + host: "" + job_states_to_report: null + job_types_to_report: null + report_template: "" # StatusErrorLink is the url that will be used for jenkins prowJobs that can't be diff --git a/prow/crier/reporters/slack/reporter.go b/prow/crier/reporters/slack/reporter.go index b725d5714941..e35c60ae3f00 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,30 @@ 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) { + var host, channel string + if cfg.Host == nil { host = DefaultHostName + } else { + host = *cfg.Host + } + if cfg.Channel != nil { + channel = *cfg.Channel } 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] + } + globalConfig := sr.config(refs) + var jobSlackConfig *v1.SlackReporterConfig + if pj.Spec.ReporterConfig != nil && pj.Spec.ReporterConfig.Slack != nil { + jobSlackConfig = pj.Spec.ReporterConfig.Slack } - return prowCfg.ReportTemplate + return &globalConfig, jobSlackConfig } func (sr *slackReporter) Report(_ context.Context, log *logrus.Entry, pj *v1.ProwJob) ([]*v1.ProwJob, *reconcile.Result, error) { @@ -87,16 +78,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 +117,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 != nil && *jobSlackConfig.Channel != "" { jobShouldReport = true } @@ -145,12 +140,15 @@ 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 + var allowedJobStates []v1.ProwJobState + if globalSlackConfig != nil && globalSlackConfig.JobStatesToReport != nil { + allowedJobStates = *globalSlackConfig.JobStatesToReport + } + if jobSlackConfig != nil && jobSlackConfig.JobStatesToReport != nil { + allowedJobStates = *jobSlackConfig.JobStatesToReport } stateShouldReport := false - for _, stateToReport := range jobStatesToReport { + for _, stateToReport := range allowedJobStates { 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..2f5728093943 100644 --- a/prow/crier/reporters/slack/reporter_test.go +++ b/prow/crier/reporters/slack/reporter_test.go @@ -22,6 +22,7 @@ import ( "github.com/sirupsen/logrus" + prowapi "k8s.io/test-infra/prow/apis/prowjobs/v1" v1 "k8s.io/test-infra/prow/apis/prowjobs/v1" "k8s.io/test-infra/prow/config" ) @@ -36,8 +37,11 @@ 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}, + Channel: pStr("whatever-channel"), + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -50,10 +54,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 +74,11 @@ 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}, + Channel: pStr("whatever-channel"), + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -82,10 +91,34 @@ func TestShouldReport(t *testing.T) { expected: true, }, { - name: "Successful Job should not report", + name: "nil job config setting negate global", 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.SuccessState}, + Channel: pStr("whatever-channel"), + }, + }, + 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: "Wrong job status should not report", + config: config.SlackReporter{ + JobTypesToReport: &[]v1.ProwJobType{v1.PostsubmitJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: &[]v1.ProwJobState{v1.PendingState}, + }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ @@ -100,14 +133,16 @@ 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{ Type: v1.PostsubmitJob, ReporterConfig: &v1.ReporterConfig{ - Slack: &v1.SlackReporterConfig{Channel: "whatever-channel"}, + Slack: &v1.SlackReporterConfig{Channel: pStr("whatever-channel")}, }, }, Status: v1.ProwJobStatus{ @@ -119,16 +154,18 @@ 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{ Type: v1.PostsubmitJob, ReporterConfig: &v1.ReporterConfig{ Slack: &v1.SlackReporterConfig{ - Channel: "whatever-channel", - JobStatesToReport: []v1.ProwJobState{v1.FailureState, v1.PendingState}, + Channel: pStr("whatever-channel"), + JobStatesToReport: &[]v1.ProwJobState{v1.FailureState, v1.PendingState}, }, }, }, @@ -141,14 +178,16 @@ 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{ Type: v1.PostsubmitJob, ReporterConfig: &v1.ReporterConfig{ - Slack: &v1.SlackReporterConfig{Channel: "whatever-channel"}, + Slack: &v1.SlackReporterConfig{Channel: pStr("whatever-channel")}, }, }, Status: v1.ProwJobStatus{ @@ -160,16 +199,18 @@ 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{ Type: v1.PostsubmitJob, ReporterConfig: &v1.ReporterConfig{ Slack: &v1.SlackReporterConfig{ - Channel: "whatever-channel", - JobStatesToReport: []v1.ProwJobState{v1.FailureState, v1.PendingState}, + Channel: pStr("whatever-channel"), + JobStatesToReport: &[]v1.ProwJobState{v1.FailureState, v1.PendingState}, }, }, }, @@ -233,8 +274,8 @@ func TestReloadsConfig(t *testing.T) { t.Error("Did expect shouldReport to be false") } - cfg.JobStatesToReport = []v1.ProwJobState{v1.FailureState} - cfg.JobTypesToReport = []v1.ProwJobType{v1.PostsubmitJob} + cfg.JobStatesToReport = &[]v1.ProwJobState{v1.FailureState} + cfg.JobTypesToReport = &[]v1.ProwJobType{v1.PostsubmitJob} if shouldReport := reporter.ShouldReport(context.Background(), logrus.NewEntry(logrus.StandardLogger()), pj); !shouldReport { t.Error("Did expect shouldReport to be true after config change") @@ -255,8 +296,10 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - Host: "global-default-host", - Channel: "global-default", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Host: pStr("global-default-host"), + Channel: pStr("global-default"), + }, }, } return config.Config{ @@ -274,11 +317,15 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - Channel: "global-default", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("global-default"), + }, }, "istio/proxy": { - Host: "global-default-host", - Channel: "org-repo-config", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Host: pStr("global-default-host"), + Channel: pStr("org-repo-config"), + }, }, } return config.Config{ @@ -302,10 +349,14 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - Channel: "global-default", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("global-default"), + }, }, "istio": { - Channel: "org-config", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("org-config"), + }, }, } return config.Config{ @@ -329,13 +380,19 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - Channel: "global-default", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("global-default"), + }, }, "istio": { - Channel: "org-config", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("org-config"), + }, }, "istio/proxy": { - Channel: "org-repo-config", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("org-repo-config"), + }, }, } return config.Config{ @@ -359,13 +416,19 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - Channel: "global-default", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("global-default"), + }, }, "istio": { - Channel: "org-config", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("org-config"), + }, }, "istio/proxy": { - Channel: "org-repo-config", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("org-repo-config"), + }, }, } return config.Config{ @@ -378,7 +441,7 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { Spec: v1.ProwJobSpec{ ReporterConfig: &v1.ReporterConfig{ Slack: &v1.SlackReporterConfig{ - Channel: "team-a", + Channel: pStr("team-a"), }, }, }, @@ -391,10 +454,14 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "istio": { - Channel: "org-config", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("org-config"), + }, }, "istio/proxy": { - Channel: "org-repo-config", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("org-repo-config"), + }, }, } return config.Config{ @@ -418,7 +485,9 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "istio/proxy": { - Channel: "org-repo-config", + SlackReporterConfig: prowapi.SlackReporterConfig{ + Channel: pStr("org-repo-config"), + }, }, } return config.Config{ @@ -449,9 +518,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 +545,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: prowapi.SlackReporterConfig{ + JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, + }, } } return config.SlackReporter{} @@ -518,10 +589,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: prowapi.SlackReporterConfig{ + JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, + Channel: pStr("emercengy"), + ReportTemplate: pStr("there you go"), + }, } } return config.SlackReporter{} @@ -536,3 +609,7 @@ func TestReportDefaultsToExtraRefs(t *testing.T) { t.Errorf("expected the channel 'emergency' to contain message 'there you go' but wasn't the case, all messages: %v", fsc.messages) } } + +func pStr(s string) *string { + return &s +} diff --git a/prow/pjutil/pjutil_test.go b/prow/pjutil/pjutil_test.go index 8bd3608c2b2a..aa214a964001 100644 --- a/prow/pjutil/pjutil_test.go +++ b/prow/pjutil/pjutil_test.go @@ -1087,7 +1087,7 @@ func TestSpecFromJobBase(t *testing.T) { jobBase: config.JobBase{ ReporterConfig: &prowapi.ReporterConfig{ Slack: &prowapi.SlackReporterConfig{ - Channel: "my-channel", + Channel: pStr("my-channel"), }, }, }, @@ -1098,9 +1098,9 @@ func TestSpecFromJobBase(t *testing.T) { if pj.ReporterConfig.Slack == nil { return errors.New("Expected ReporterConfig.Slack to be non-nil") } - if pj.ReporterConfig.Slack.Channel != "my-channel" { + if *pj.ReporterConfig.Slack.Channel != "my-channel" { return fmt.Errorf("Expected pj.ReporterConfig.Slack.Channel to be \"my-channel\", was %q", - pj.ReporterConfig.Slack.Channel) + *pj.ReporterConfig.Slack.Channel) } return nil }, @@ -1174,3 +1174,7 @@ func TestPeriodicSpec(t *testing.T) { } } } + +func pStr(str string) *string { + return &str +} From 09827e9315529566345c8b0badea5c7bb8dec809 Mon Sep 17 00:00:00 2001 From: Chao Dai Date: Thu, 24 Jun 2021 11:21:05 -0700 Subject: [PATCH 2/2] Switch back from pointers and add fuzz tests --- prow/apis/prowjobs/v1/types.go | 14 +- prow/apis/prowjobs/v1/types_test.go | 30 ++++ .../apis/prowjobs/v1/zz_generated.deepcopy.go | 23 +-- prow/config/config.go | 13 +- prow/config/config_test.go | 30 ++-- prow/config/prow-config-documented.yaml | 12 +- prow/crier/reporters/slack/reporter.go | 34 ++-- prow/crier/reporters/slack/reporter_test.go | 167 ++++++++++-------- prow/pjutil/pjutil_test.go | 10 +- 9 files changed, 172 insertions(+), 161 deletions(-) diff --git a/prow/apis/prowjobs/v1/types.go b/prow/apis/prowjobs/v1/types.go index 91b6f3c89f1a..4df22bae90ed 100644 --- a/prow/apis/prowjobs/v1/types.go +++ b/prow/apis/prowjobs/v1/types.go @@ -286,10 +286,10 @@ type ReporterConfig struct { } type SlackReporterConfig struct { - Host *string `json:"host,omitempty"` - Channel *string `json:"channel,omitempty"` - JobStatesToReport *[]ProwJobState `json:"job_states_to_report,omitempty"` - ReportTemplate *string `json:"report_template,omitempty"` + Host string `json:"host,omitempty"` + Channel string `json:"channel,omitempty"` + JobStatesToReport []ProwJobState `json:"job_states_to_report,omitempty"` + ReportTemplate string `json:"report_template,omitempty"` } func (src *SlackReporterConfig) ApplyDefault(def *SlackReporterConfig) *SlackReporterConfig { @@ -306,16 +306,16 @@ func (src *SlackReporterConfig) ApplyDefault(def *SlackReporterConfig) *SlackRep return &merged } - if merged.Channel == nil { + if merged.Channel == "" { merged.Channel = def.Channel } - if merged.Host == nil { + if merged.Host == "" { merged.Host = def.Host } if merged.JobStatesToReport == nil { merged.JobStatesToReport = def.JobStatesToReport } - if merged.ReportTemplate == nil { + if merged.ReportTemplate == "" { merged.ReportTemplate = def.ReportTemplate } return &merged 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/apis/prowjobs/v1/zz_generated.deepcopy.go b/prow/apis/prowjobs/v1/zz_generated.deepcopy.go index 77cf060d723d..b9b4d2eb386c 100644 --- a/prow/apis/prowjobs/v1/zz_generated.deepcopy.go +++ b/prow/apis/prowjobs/v1/zz_generated.deepcopy.go @@ -547,29 +547,10 @@ func (in *Resources) DeepCopy() *Resources { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SlackReporterConfig) DeepCopyInto(out *SlackReporterConfig) { *out = *in - if in.Host != nil { - in, out := &in.Host, &out.Host - *out = new(string) - **out = **in - } - if in.Channel != nil { - in, out := &in.Channel, &out.Channel - *out = new(string) - **out = **in - } if in.JobStatesToReport != nil { in, out := &in.JobStatesToReport, &out.JobStatesToReport - *out = new([]ProwJobState) - if **in != nil { - in, out := *in, *out - *out = make([]ProwJobState, len(*in)) - copy(*out, *in) - } - } - if in.ReportTemplate != nil { - in, out := &in.ReportTemplate, &out.ReportTemplate - *out = new(string) - **out = **in + *out = make([]ProwJobState, len(*in)) + copy(*out, *in) } return } diff --git a/prow/config/config.go b/prow/config/config.go index a0b7f019956e..50202606516a 100644 --- a/prow/config/config.go +++ b/prow/config/config.go @@ -1042,8 +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"` - prowapi.SlackReporterConfig + JobTypesToReport []prowapi.ProwJobType `json:"job_types_to_report,omitempty"` + prowapi.SlackReporterConfig `json:",inline"` } // SlackReporterConfigs represents the config for the Slack reporter(s). @@ -1068,17 +1068,16 @@ func (cfg SlackReporterConfigs) GetSlackReporter(refs *prowapi.Refs) SlackReport func (cfg *SlackReporter) DefaultAndValidate() error { // Default ReportTemplate - if cfg.ReportTemplate == nil { - defaultTemplate := `Job {{.Spec.Job}} of type {{.Spec.Type}} ended with state {{.Status.State}}. <{{.Status.URL}}|View logs>` - cfg.ReportTemplate = &defaultTemplate + if cfg.ReportTemplate == "" { + cfg.ReportTemplate = `Job {{.Spec.Job}} of type {{.Spec.Type}} ended with state {{.Status.State}}. <{{.Status.URL}}|View logs>` } - if cfg.Channel == nil || *cfg.Channel == "" { + if cfg.Channel == "" { return errors.New("channel must be set") } // Validate ReportTemplate - tmpl, err := template.New("").Parse(*cfg.ReportTemplate) + tmpl, err := template.New("").Parse(cfg.ReportTemplate) if err != nil { return fmt.Errorf("failed to parse template: %v", err) } diff --git a/prow/config/config_test.go b/prow/config/config_test.go index 23c61299af1f..c91738c79b9c 100644 --- a/prow/config/config_test.go +++ b/prow/config/config_test.go @@ -3330,8 +3330,8 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "*": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("my-channel"), + SlackReporterConfig: prowjobv1.SlackReporterConfig{ + Channel: "my-channel", }, }, } @@ -3348,8 +3348,8 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "istio/proxy": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("my-channel"), + SlackReporterConfig: prowjobv1.SlackReporterConfig{ + Channel: "my-channel", }, }, } @@ -3366,8 +3366,8 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "proxy": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("my-channel"), + SlackReporterConfig: prowjobv1.SlackReporterConfig{ + Channel: "my-channel", }, }, } @@ -3384,7 +3384,7 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "*": { - JobTypesToReport: &[]prowapi.ProwJobType{"presubmit"}, + JobTypesToReport: []prowapi.ProwJobType{"presubmit"}, }, } return Config{ @@ -3412,9 +3412,9 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "*": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("my-channel"), - ReportTemplate: pStr("{{ if .Spec.Name}}"), + SlackReporterConfig: prowjobv1.SlackReporterConfig{ + Channel: "my-channel", + ReportTemplate: "{{ if .Spec.Name}}", }, }, } @@ -3431,9 +3431,9 @@ func TestSlackReporterValidation(t *testing.T) { config: func() Config { slackCfg := map[string]SlackReporter{ "*": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("my-channel"), - ReportTemplate: pStr("{{ .Undef}}"), + SlackReporterConfig: prowjobv1.SlackReporterConfig{ + Channel: "my-channel", + ReportTemplate: "{{ .Undef}}", }, }, } @@ -3455,10 +3455,10 @@ func TestSlackReporterValidation(t *testing.T) { } if tc.successExpected { for _, config := range cfg.SlackReporterConfigs { - if *config.ReportTemplate == "" { + if config.ReportTemplate == "" { t.Errorf("expected default ReportTemplate to be set") } - if *config.Channel == "" { + if config.Channel == "" { t.Errorf("expected Channel to be required") } } diff --git a/prow/config/prow-config-documented.yaml b/prow/config/prow-config-documented.yaml index 1593b87e8137..49575bd3e3d4 100644 --- a/prow/config/prow-config-documented.yaml +++ b/prow/config/prow-config-documented.yaml @@ -947,11 +947,13 @@ sinker: terminated_pod_ttl: 0s slack_reporter_configs: "": - channel: "" - host: "" - job_states_to_report: null - job_types_to_report: null - report_template: "" + channel: ' ' + host: ' ' + job_states_to_report: + - "" + job_types_to_report: + - "" + report_template: ' ' # StatusErrorLink is the url that will be used for jenkins prowJobs that can't be diff --git a/prow/crier/reporters/slack/reporter.go b/prow/crier/reporters/slack/reporter.go index e35c60ae3f00..b835fb42ab7e 100644 --- a/prow/crier/reporters/slack/reporter.go +++ b/prow/crier/reporters/slack/reporter.go @@ -48,14 +48,9 @@ type slackReporter struct { } func hostAndChannel(cfg *v1.SlackReporterConfig) (string, string) { - var host, channel string - if cfg.Host == nil { + host, channel := cfg.Host, cfg.Channel + if host == "" { host = DefaultHostName - } else { - host = *cfg.Host - } - if cfg.Channel != nil { - channel = *cfg.Channel } return host, channel } @@ -92,7 +87,7 @@ func (sr *slackReporter) report(log *logrus.Entry, pj *v1.ProwJob) error { return fmt.Errorf("host '%s' not supported", host) } b := &bytes.Buffer{} - tmpl, err := template.New("").Parse(*jobSlackConfig.ReportTemplate) + 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,7 +116,7 @@ func (sr *slackReporter) ShouldReport(_ context.Context, logger *logrus.Entry, p var typeShouldReport bool if globalSlackConfig.JobTypesToReport != nil { - for _, tp := range *globalSlackConfig.JobTypesToReport { + for _, tp := range globalSlackConfig.JobTypesToReport { if tp == pj.Spec.Type { typeShouldReport = true break @@ -132,7 +127,7 @@ func (sr *slackReporter) ShouldReport(_ context.Context, logger *logrus.Entry, p // If a user specifically put a channel on their job, they want // it to be reported regardless of the job types setting. var jobShouldReport bool - if jobSlackConfig != nil && jobSlackConfig.Channel != nil && *jobSlackConfig.Channel != "" { + if jobSlackConfig != nil && jobSlackConfig.Channel != "" { jobShouldReport = true } @@ -140,18 +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. - var allowedJobStates []v1.ProwJobState - if globalSlackConfig != nil && globalSlackConfig.JobStatesToReport != nil { - allowedJobStates = *globalSlackConfig.JobStatesToReport - } - if jobSlackConfig != nil && jobSlackConfig.JobStatesToReport != nil { - allowedJobStates = *jobSlackConfig.JobStatesToReport - } - stateShouldReport := false - for _, stateToReport := range allowedJobStates { - 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 2f5728093943..1abe7f08fa45 100644 --- a/prow/crier/reporters/slack/reporter_test.go +++ b/prow/crier/reporters/slack/reporter_test.go @@ -22,7 +22,6 @@ import ( "github.com/sirupsen/logrus" - prowapi "k8s.io/test-infra/prow/apis/prowjobs/v1" v1 "k8s.io/test-infra/prow/apis/prowjobs/v1" "k8s.io/test-infra/prow/config" ) @@ -37,10 +36,9 @@ func TestShouldReport(t *testing.T) { { name: "Presubmit Job should report", config: config.SlackReporter{ - JobTypesToReport: &[]v1.ProwJobType{v1.PresubmitJob}, + JobTypesToReport: []v1.ProwJobType{v1.PresubmitJob}, SlackReporterConfig: v1.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, - Channel: pStr("whatever-channel"), + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, }, }, pj: &v1.ProwJob{ @@ -54,11 +52,11 @@ func TestShouldReport(t *testing.T) { expected: true, }, { - name: "Wrong job type should not report", + name: "Wrong job type should not report", config: config.SlackReporter{ - JobTypesToReport: &[]v1.ProwJobType{v1.PostsubmitJob}, + JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, SlackReporterConfig: v1.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, }, }, pj: &v1.ProwJob{ @@ -74,10 +72,9 @@ func TestShouldReport(t *testing.T) { { name: "Successful Job should report", config: config.SlackReporter{ - JobTypesToReport: &[]v1.ProwJobType{v1.PostsubmitJob}, + JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, SlackReporterConfig: v1.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, - Channel: pStr("whatever-channel"), + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, }, }, pj: &v1.ProwJob{ @@ -91,19 +88,18 @@ func TestShouldReport(t *testing.T) { expected: true, }, { - name: "nil job config setting negate global", + name: "Empty job config settings negate global", config: config.SlackReporter{ - JobTypesToReport: &[]v1.ProwJobType{v1.PostsubmitJob}, + JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, SlackReporterConfig: v1.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, - Channel: pStr("whatever-channel"), + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ Type: v1.PostsubmitJob, ReporterConfig: &v1.ReporterConfig{ - Slack: &v1.SlackReporterConfig{JobStatesToReport: &[]v1.ProwJobState{}}, + Slack: &v1.SlackReporterConfig{JobStatesToReport: []v1.ProwJobState{}}, }, }, Status: v1.ProwJobStatus{ @@ -113,11 +109,32 @@ func TestShouldReport(t *testing.T) { expected: false, }, { - name: "Wrong job status should not report", + name: "Nil job config settings does not negate global", config: config.SlackReporter{ - JobTypesToReport: &[]v1.ProwJobType{v1.PostsubmitJob}, + JobTypesToReport: []v1.ProwJobType{v1.PostsubmitJob}, SlackReporterConfig: v1.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.PendingState}, + 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}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.PendingState}, }, }, pj: &v1.ProwJob{ @@ -133,16 +150,16 @@ func TestShouldReport(t *testing.T) { { name: "Job with channel config should ignore the JobTypesToReport config", config: config.SlackReporter{ - JobTypesToReport: &[]v1.ProwJobType{}, + JobTypesToReport: []v1.ProwJobType{}, SlackReporterConfig: v1.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ Type: v1.PostsubmitJob, ReporterConfig: &v1.ReporterConfig{ - Slack: &v1.SlackReporterConfig{Channel: pStr("whatever-channel")}, + Slack: &v1.SlackReporterConfig{Channel: "whatever-channel"}, }, }, Status: v1.ProwJobStatus{ @@ -154,9 +171,9 @@ func TestShouldReport(t *testing.T) { { name: "JobStatesToReport in Job config should override the one in Prow config", config: config.SlackReporter{ - JobTypesToReport: &[]v1.ProwJobType{}, + JobTypesToReport: []v1.ProwJobType{}, SlackReporterConfig: v1.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, }, }, pj: &v1.ProwJob{ @@ -164,8 +181,8 @@ func TestShouldReport(t *testing.T) { Type: v1.PostsubmitJob, ReporterConfig: &v1.ReporterConfig{ Slack: &v1.SlackReporterConfig{ - Channel: pStr("whatever-channel"), - JobStatesToReport: &[]v1.ProwJobState{v1.FailureState, v1.PendingState}, + Channel: "whatever-channel", + JobStatesToReport: []v1.ProwJobState{v1.FailureState, v1.PendingState}, }, }, }, @@ -178,16 +195,16 @@ 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{}, + JobTypesToReport: []v1.ProwJobType{}, SlackReporterConfig: v1.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, }, }, pj: &v1.ProwJob{ Spec: v1.ProwJobSpec{ Type: v1.PostsubmitJob, ReporterConfig: &v1.ReporterConfig{ - Slack: &v1.SlackReporterConfig{Channel: pStr("whatever-channel")}, + Slack: &v1.SlackReporterConfig{Channel: "whatever-channel"}, }, }, Status: v1.ProwJobStatus{ @@ -199,9 +216,9 @@ 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{}, + JobTypesToReport: []v1.ProwJobType{}, SlackReporterConfig: v1.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, }, }, pj: &v1.ProwJob{ @@ -209,8 +226,8 @@ func TestShouldReport(t *testing.T) { Type: v1.PostsubmitJob, ReporterConfig: &v1.ReporterConfig{ Slack: &v1.SlackReporterConfig{ - Channel: pStr("whatever-channel"), - JobStatesToReport: &[]v1.ProwJobState{v1.FailureState, v1.PendingState}, + Channel: "whatever-channel", + JobStatesToReport: []v1.ProwJobState{v1.FailureState, v1.PendingState}, }, }, }, @@ -274,8 +291,8 @@ func TestReloadsConfig(t *testing.T) { t.Error("Did expect shouldReport to be false") } - cfg.JobStatesToReport = &[]v1.ProwJobState{v1.FailureState} - cfg.JobTypesToReport = &[]v1.ProwJobType{v1.PostsubmitJob} + cfg.JobStatesToReport = []v1.ProwJobState{v1.FailureState} + cfg.JobTypesToReport = []v1.ProwJobType{v1.PostsubmitJob} if shouldReport := reporter.ShouldReport(context.Background(), logrus.NewEntry(logrus.StandardLogger()), pj); !shouldReport { t.Error("Did expect shouldReport to be true after config change") @@ -296,9 +313,9 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Host: pStr("global-default-host"), - Channel: pStr("global-default"), + SlackReporterConfig: v1.SlackReporterConfig{ + Host: "global-default-host", + Channel: "global-default", }, }, } @@ -317,14 +334,14 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("global-default"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "global-default", }, }, "istio/proxy": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Host: pStr("global-default-host"), - Channel: pStr("org-repo-config"), + SlackReporterConfig: v1.SlackReporterConfig{ + Host: "global-default-host", + Channel: "org-repo-config", }, }, } @@ -349,13 +366,13 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("global-default"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "global-default", }, }, "istio": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("org-config"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-config", }, }, } @@ -380,18 +397,18 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("global-default"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "global-default", }, }, "istio": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("org-config"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-config", }, }, "istio/proxy": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("org-repo-config"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-repo-config", }, }, } @@ -416,18 +433,18 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "*": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("global-default"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "global-default", }, }, "istio": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("org-config"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-config", }, }, "istio/proxy": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("org-repo-config"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-repo-config", }, }, } @@ -441,7 +458,7 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { Spec: v1.ProwJobSpec{ ReporterConfig: &v1.ReporterConfig{ Slack: &v1.SlackReporterConfig{ - Channel: pStr("team-a"), + Channel: "team-a", }, }, }, @@ -454,13 +471,13 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "istio": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("org-config"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-config", }, }, "istio/proxy": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("org-repo-config"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-repo-config", }, }, } @@ -485,8 +502,8 @@ func TestUsesChannelOverrideFromJob(t *testing.T) { config: func() config.Config { slackCfg := map[string]config.SlackReporter{ "istio/proxy": { - SlackReporterConfig: prowapi.SlackReporterConfig{ - Channel: pStr("org-repo-config"), + SlackReporterConfig: v1.SlackReporterConfig{ + Channel: "org-repo-config", }, }, } @@ -545,9 +562,9 @@ func TestShouldReportDefaultsToExtraRefs(t *testing.T) { config: func(r *v1.Refs) config.SlackReporter { if r.Org == "org" { return config.SlackReporter{ - JobTypesToReport: &[]v1.ProwJobType{v1.PeriodicJob}, - SlackReporterConfig: prowapi.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, + JobTypesToReport: []v1.ProwJobType{v1.PeriodicJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, }, } } @@ -589,11 +606,11 @@ func TestReportDefaultsToExtraRefs(t *testing.T) { config: func(r *v1.Refs) config.SlackReporter { if r.Org == "org" { return config.SlackReporter{ - JobTypesToReport: &[]v1.ProwJobType{v1.PeriodicJob}, - SlackReporterConfig: prowapi.SlackReporterConfig{ - JobStatesToReport: &[]v1.ProwJobState{v1.SuccessState}, - Channel: pStr("emercengy"), - ReportTemplate: pStr("there you go"), + JobTypesToReport: []v1.ProwJobType{v1.PeriodicJob}, + SlackReporterConfig: v1.SlackReporterConfig{ + JobStatesToReport: []v1.ProwJobState{v1.SuccessState}, + Channel: "emercengy", + ReportTemplate: "there you go", }, } } @@ -609,7 +626,3 @@ func TestReportDefaultsToExtraRefs(t *testing.T) { t.Errorf("expected the channel 'emergency' to contain message 'there you go' but wasn't the case, all messages: %v", fsc.messages) } } - -func pStr(s string) *string { - return &s -} diff --git a/prow/pjutil/pjutil_test.go b/prow/pjutil/pjutil_test.go index aa214a964001..8bd3608c2b2a 100644 --- a/prow/pjutil/pjutil_test.go +++ b/prow/pjutil/pjutil_test.go @@ -1087,7 +1087,7 @@ func TestSpecFromJobBase(t *testing.T) { jobBase: config.JobBase{ ReporterConfig: &prowapi.ReporterConfig{ Slack: &prowapi.SlackReporterConfig{ - Channel: pStr("my-channel"), + Channel: "my-channel", }, }, }, @@ -1098,9 +1098,9 @@ func TestSpecFromJobBase(t *testing.T) { if pj.ReporterConfig.Slack == nil { return errors.New("Expected ReporterConfig.Slack to be non-nil") } - if *pj.ReporterConfig.Slack.Channel != "my-channel" { + if pj.ReporterConfig.Slack.Channel != "my-channel" { return fmt.Errorf("Expected pj.ReporterConfig.Slack.Channel to be \"my-channel\", was %q", - *pj.ReporterConfig.Slack.Channel) + pj.ReporterConfig.Slack.Channel) } return nil }, @@ -1174,7 +1174,3 @@ func TestPeriodicSpec(t *testing.T) { } } } - -func pStr(str string) *string { - return &str -}