Skip to content

Commit

Permalink
Validate Gerrit label on jobs which skip reporting
Browse files Browse the repository at this point in the history 
It is now a config error to configure gerrit report label to a non-empty
string for jobs which skip reporting, i.e. have `skip_report` field set.

Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
  • Loading branch information
@adshmh
adshmh committed May 9, 2020
1 parent a3212b1 commit b3b0bf5
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 4 deletions.
4 changes: 4 additions & 0 deletions prow/config/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ go_test(
deps = [
"//prow/apis/prowjobs/v1:go_default_library",
"//prow/config/secret:go_default_library",
"//prow/gerrit/client:go_default_library",
"//prow/git/localgit:go_default_library",
"//prow/git/v2:go_default_library",
"//prow/github:go_default_library",
Expand All @@ -31,9 +32,11 @@ go_test(
"//prow/labels:go_default_library",
"//prow/pod-utils/decorate:go_default_library",
"//prow/pod-utils/downwardapi:go_default_library",
"@com_github_google_go_cmp//cmp:go_default_library",
"@com_github_tektoncd_pipeline//pkg/apis/pipeline/v1alpha1:go_default_library",
"@io_k8s_api//core/v1:go_default_library",
"@io_k8s_apimachinery//pkg/util/diff:go_default_library",
"@io_k8s_apimachinery//pkg/util/errors:go_default_library",
"@io_k8s_apimachinery//pkg/util/sets:go_default_library",
"@io_k8s_utils//pointer:go_default_library",
],
Expand All @@ -52,6 +55,7 @@ go_library(
importpath = "k8s.io/test-infra/prow/config",
deps = [
"//prow/apis/prowjobs/v1:go_default_library",
"//prow/gerrit/client:go_default_library",
"//prow/git/v2:go_default_library",
"//prow/github:go_default_library",
"//prow/kube:go_default_library",
Expand Down
16 changes: 12 additions & 4 deletions prow/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
"sigs.k8s.io/yaml"

prowapi "k8s.io/test-infra/prow/apis/prowjobs/v1"
gerrit "k8s.io/test-infra/prow/gerrit/client"
"k8s.io/test-infra/prow/git/v2"
"k8s.io/test-infra/prow/github"
"k8s.io/test-infra/prow/kube"
Expand Down Expand Up @@ -1274,7 +1275,6 @@ func validateJobBase(v JobBase, jobType prowapi.ProwJobType, podNamespace string
// validatePresubmits validates the presubmits for one repo
func validatePresubmits(presubmits []Presubmit, podNamespace string) error {
validPresubmits := map[string][]Presubmit{}

var errs []error
for _, ps := range presubmits {
// Checking that no duplicate job in prow config exists on the same branch.
Expand All @@ -1297,7 +1297,7 @@ func validatePresubmits(presubmits []Presubmit, podNamespace string) error {
if err := validateTriggering(ps); err != nil {
errs = append(errs, err)
}
if err := validateReporting(ps.Reporter); err != nil {
if err := validateReporting(ps.JobBase, ps.Reporter); err != nil {
errs = append(errs, fmt.Errorf("invalid presubmit job %s: %v", ps.Name, err))
}
validPresubmits[ps.Name] = append(validPresubmits[ps.Name], ps)
Expand Down Expand Up @@ -1353,7 +1353,7 @@ func validatePostsubmits(postsubmits []Postsubmit, podNamespace string) error {
if err := validateJobBase(ps.JobBase, prowapi.PostsubmitJob, podNamespace); err != nil {
errs = append(errs, fmt.Errorf("invalid postsubmit job %s: %v", ps.Name, err))
}
if err := validateReporting(ps.Reporter); err != nil {
if err := validateReporting(ps.JobBase, ps.Reporter); err != nil {
errs = append(errs, fmt.Errorf("invalid postsubmit job %s: %v", ps.Name, err))
}
validPostsubmits[ps.Name] = append(validPostsubmits[ps.Name], ps)
Expand Down Expand Up @@ -1915,10 +1915,18 @@ func validateTriggering(job Presubmit) error {
return nil
}

func validateReporting(r Reporter) error {
func validateReporting(j JobBase, r Reporter) error {
if !r.SkipReport && r.Context == "" {
return errors.New("job is set to report but has no context configured")
}
if !r.SkipReport {
return nil
}
for label, value := range j.Labels {
if label == gerrit.GerritReportLabel && value != "" {
return fmt.Errorf("Gerrit report label %s set to non-empty string but job is configured to skip reporting.", label)
}
}
return nil
}

Expand Down
79 changes: 79 additions & 0 deletions prow/config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,18 @@ import (
"text/template"
"time"

"github.com/google/go-cmp/cmp"
pipelinev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/diff"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/sets"
utilpointer "k8s.io/utils/pointer"

prowapi "k8s.io/test-infra/prow/apis/prowjobs/v1"
prowjobv1 "k8s.io/test-infra/prow/apis/prowjobs/v1"
"k8s.io/test-infra/prow/config/secret"
gerrit "k8s.io/test-infra/prow/gerrit/client"
"k8s.io/test-infra/prow/github"
"k8s.io/test-infra/prow/github/fakegithub"
"k8s.io/test-infra/prow/kube"
Expand Down Expand Up @@ -1274,6 +1277,82 @@ func TestValidateRefs(t *testing.T) {
}
}

func TestValidateReportingWithGerritLabel(t *testing.T) {
cases := []struct {
name string
labels map[string]string
reporter Reporter
expected error
}{
{
name: "no errors if job is set to report",
reporter: Reporter{
Context: "context",
},
labels: map[string]string{
gerrit.GerritReportLabel: "label",
},
},
{
name: "no errors if Gerrit report label is not defined",
reporter: Reporter{SkipReport: true},
labels: map[string]string{
"label": "value",
},
},
{
name: "no errors if job is set to skip report and Gerrit report label is empty",
reporter: Reporter{SkipReport: true},
labels: map[string]string{
gerrit.GerritReportLabel: "",
},
},
{
name: "error if job is set to skip report and Gerrit report label is set to non-empty",
reporter: Reporter{SkipReport: true},
labels: map[string]string{
gerrit.GerritReportLabel: "label",
},
expected: fmt.Errorf("Gerrit report label %s set to non-empty string but job is configured to skip reporting.", gerrit.GerritReportLabel),
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
base := JobBase{
Name: "test-job",
Labels: tc.labels,
}
presubmits := []Presubmit{
{
JobBase: base,
Reporter: tc.reporter,
},
}
var expected error
if tc.expected != nil {
expected = fmt.Errorf("invalid presubmit job %s: %v", "test-job", tc.expected)
}
if err := validatePresubmits(presubmits, "default-namespace"); !reflect.DeepEqual(err, utilerrors.NewAggregate([]error{expected})) {
t.Errorf("did not get expected validation result:\n%v", cmp.Diff(expected, err))
}

postsubmits := []Postsubmit{
{
JobBase: base,
Reporter: tc.reporter,
},
}
if tc.expected != nil {
expected = fmt.Errorf("invalid postsubmit job %s: %v", "test-job", tc.expected)
}
if err := validatePostsubmits(postsubmits, "default-namespace"); !reflect.DeepEqual(err, utilerrors.NewAggregate([]error{expected})) {
t.Errorf("did not get expected validation result:\n%v", cmp.Diff(expected, err))
}
})
}
}

// integration test for fake config loading
func TestValidConfigLoading(t *testing.T) {
var testCases = []struct {
Expand Down

0 comments on commit b3b0bf5

Please sign in to comment.