forked from kubernetes/test-infra
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
tests: ensure that pre-submits get additional reviews
Blocking pre-submit jobs must be for stable, important features and must always run. Non-blocking pre-submit jobs should only be run automatically if they meet the criteria outlined in kubernetes/community#8196. To ensure that this is considered when defining pre-submit jobs, they need to be listed in `config/tests/jobs/policy/presubmit-jobs.yaml`. The OWNERS file in that new directory ensures that relevant reviewers need to approve. `make update-config-fixture` re-generates that file to the expected state for inclusion in a PR.
- Loading branch information
Showing
8 changed files
with
406 additions
and
3 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# See the OWNERS docs at https://go.k8s.io/owners | ||
|
||
options: | ||
no_parent_owners: true | ||
|
||
reviewers: | ||
- test-infra-oncall # oncall | ||
- BenTheElder # lead | ||
- aojea # lead | ||
approvers: | ||
- test-infra-oncall # oncall | ||
- BenTheElder # lead | ||
- aojea # lead | ||
|
||
labels: | ||
- sig/testing | ||
- sig/infra |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
The project has certain guidelines around jobs which are meant to ensure that | ||
there's a balance between test coverage and costs for running the CI. For | ||
example, non-blocking jobs that get trigger automatically for PRs should be | ||
used judiciously. | ||
|
||
Because SIG leads are not necessarily familiar with those policies, SIG Testing | ||
and SIG Infra need to be involved before merging jobs that fall into those | ||
sensitive areas. This is achieved with tests and additional files in this | ||
directory and a separate OWNERS file. | ||
|
||
To check whether jobs are okay, run the Go tests in this directory. | ||
If tests fail, re-run with the `UPDATE_FIXTURE_DATA=true` env variable | ||
and include the modified files in the PR which updates the jobs. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,257 @@ | ||
/* | ||
Copyright 2018 The Kubernetes Authors. | ||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
http://www.apache.org/licenses/LICENSE-2.0 | ||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package policy | ||
|
||
// This file validates Kubernetes's jobs configs against policies. | ||
|
||
import ( | ||
"bytes" | ||
"flag" | ||
"fmt" | ||
"os" | ||
"path/filepath" | ||
"slices" | ||
"sort" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/google/go-cmp/cmp" | ||
"github.com/google/go-cmp/cmp/cmpopts" | ||
yaml "sigs.k8s.io/yaml/goyaml.v3" | ||
|
||
cfg "sigs.k8s.io/prow/pkg/config" | ||
) | ||
|
||
var configPath = flag.String("config", "../../../../config/prow/config.yaml", "Path to prow config") | ||
var jobConfigPath = flag.String("job-config", "../../../jobs", "Path to prow job config") | ||
var deckPath = flag.String("deck-path", "https://prow.k8s.io", "Path to deck") | ||
var bucket = flag.String("bucket", "kubernetes-ci-logs", "Gcs bucket for log upload") | ||
var k8sProw = flag.Bool("k8s-prow", true, "If the config is for k8s prow cluster") | ||
|
||
// Loaded at TestMain. | ||
var c *cfg.Config | ||
|
||
func TestMain(m *testing.M) { | ||
flag.Parse() | ||
if *configPath == "" { | ||
fmt.Println("--config must set") | ||
os.Exit(1) | ||
} | ||
|
||
conf, err := cfg.Load(*configPath, *jobConfigPath, nil, "") | ||
if err != nil { | ||
fmt.Printf("Could not load config: %v", err) | ||
os.Exit(1) | ||
} | ||
c = conf | ||
|
||
os.Exit(m.Run()) | ||
} | ||
|
||
func TestKubernetesPresubmitJobs(t *testing.T) { | ||
jobs := c.AllStaticPresubmits([]string{"kubernetes/kubernetes"}) | ||
var expected presubmitJobs | ||
|
||
for _, job := range jobs { | ||
if !job.AlwaysRun && job.RunIfChanged == "" { | ||
// Manually triggered, no additional review needed. | ||
continue | ||
} | ||
|
||
// Mirror those attributes of the job which must trigger additional reviews | ||
// or are needed to identify the job. | ||
j := presubmitJob{ | ||
Name: job.Name, | ||
SkipBranches: job.SkipBranches, | ||
Branches: job.Branches, | ||
|
||
RunIfChanged: job.RunIfChanged, | ||
SkipIfOnlyChanged: job.SkipIfOnlyChanged, | ||
} | ||
|
||
// This uses separate top-level fields instead of job attributes to | ||
// make it more obvious when run_if_changed is used. | ||
if job.AlwaysRun { | ||
expected.AlwaysRun = append(expected.AlwaysRun, j) | ||
} else { | ||
expected.RunIfChanged = append(expected.RunIfChanged, j) | ||
|
||
if !job.Optional { | ||
// Absolute path is more user-friendly than ../../config/... | ||
t.Errorf("Policy violation: %s in %s should use `optional: true` or `alwaysRun: true`.", job.Name, maybeAbsPath(job.SourcePath)) | ||
} | ||
} | ||
|
||
} | ||
expected.Normalize() | ||
|
||
// Encode the expected content. | ||
var expectedData bytes.Buffer | ||
if _, err := expectedData.Write([]byte(`# AUTOGENERATED by "UPDATE_FIXTURE_DATA=true go test ./config/tests/jobs". DO NOT EDIT! | ||
`)); err != nil { | ||
t.Fatalf("unexpected error writing into buffer: %v", err) | ||
} | ||
|
||
encoder := yaml.NewEncoder(&expectedData) | ||
encoder.SetIndent(4) | ||
if err := encoder.Encode(expected); err != nil { | ||
t.Fatalf("unexpected error encoding %s: %v", presubmitsFile, err) | ||
} | ||
|
||
// Compare. This proceeds on read or decoding errors because | ||
// the file might get re-generated below. | ||
var actual presubmitJobs | ||
actualData, err := os.ReadFile(presubmitsFile) | ||
if err != nil && !os.IsNotExist(err) { | ||
t.Errorf("unexpected error: %v", err) | ||
} | ||
if err := yaml.Unmarshal(actualData, &actual); err != nil { | ||
t.Errorf("unexpected error decoding %s: %v", presubmitsFile, err) | ||
} | ||
|
||
// First check the in-memory structs. The diff is nicer for them (more context). | ||
diff := cmp.Diff(actual, expected) | ||
if diff == "" { | ||
// Next check the encoded data. This should only be different on test updates. | ||
diff = cmp.Diff(string(actualData), expectedData.String(), cmpopts.AcyclicTransformer("SplitLines", func(s string) []string { | ||
return strings.Split(s, "\n") | ||
})) | ||
} | ||
|
||
if diff != "" { | ||
if value, _ := os.LookupEnv("UPDATE_FIXTURE_DATA"); value == "true" { | ||
if err := os.WriteFile(presubmitsFile, expectedData.Bytes(), 0644); err != nil { | ||
t.Fatalf("unexpected error: %v", err) | ||
} | ||
t.Logf(` | ||
%s was out-dated. Updated as requested with the following changes (- actual, + expected): | ||
%s | ||
`, maybeAbsPath(presubmitsFile), diff) | ||
} else { | ||
t.Errorf(` | ||
%s is out-dated. Detected differences (- actual, + expected): | ||
%s | ||
Blocking pre-submit jobs must be for stable, important features. | ||
Non-blocking pre-submit jobs should only be run automatically if they meet | ||
the criteria outlined in https://github.com/kubernetes/community/pull/8196. | ||
To ensure that this is considered when defining pre-submit jobs, they | ||
need to be listed in %s. If the pre-submit job is really needed, | ||
re-run the test with UPDATE_FIXTURE_DATA=true and include the modified | ||
file. The following command can be used: | ||
make update-config-fixture | ||
`, presubmitsFile, diff, presubmitsFile) | ||
} | ||
} | ||
} | ||
|
||
// presubmitsFile contains the following struct. | ||
const presubmitsFile = "presubmit-jobs.yaml" | ||
|
||
type presubmitJobs struct { | ||
AlwaysRun []presubmitJob `yaml:"always_run"` | ||
RunIfChanged []presubmitJob `yaml:"run_if_changed"` | ||
} | ||
type presubmitJob struct { | ||
Name string `yaml:"name"` | ||
SkipBranches []string `yaml:"skip_branches,omitempty"` | ||
Branches []string `yaml:"branches,omitempty"` | ||
RunIfChanged string `yaml:"run_if_changed,omitempty"` | ||
SkipIfOnlyChanged string `yaml:"skip_if_only_changed,omitempty"` | ||
} | ||
|
||
func (p *presubmitJobs) Normalize() { | ||
sortJobs(&p.AlwaysRun) | ||
sortJobs(&p.RunIfChanged) | ||
} | ||
|
||
func sortJobs(jobs *[]presubmitJob) { | ||
for _, job := range *jobs { | ||
sort.Strings(job.SkipBranches) | ||
sort.Strings(job.Branches) | ||
} | ||
sort.Slice(*jobs, func(i, j int) bool { | ||
switch strings.Compare((*jobs)[i].Name, (*jobs)[j].Name) { | ||
case -1: | ||
return true | ||
case 1: | ||
return false | ||
} | ||
switch slices.Compare((*jobs)[i].SkipBranches, (*jobs)[j].SkipBranches) { | ||
case -1: | ||
return true | ||
case 1: | ||
return false | ||
} | ||
switch slices.Compare((*jobs)[i].Branches, (*jobs)[j].Branches) { | ||
case -1: | ||
return true | ||
case 1: | ||
return false | ||
} | ||
return false | ||
}) | ||
|
||
// If a job has the same settings regardless of the branch, then | ||
// we can reduce to a single entry without the branch info. | ||
shorterJobs := make([]presubmitJob, 0, len(*jobs)) | ||
for i := 0; i < len(*jobs); { | ||
job := (*jobs)[i] | ||
job.Branches = nil | ||
job.SkipBranches = nil | ||
|
||
if sameSettings(*jobs, job) { | ||
shorterJobs = append(shorterJobs, job) | ||
// Fast-forward to next job. | ||
for i < len(*jobs) && (*jobs)[i].Name == job.Name { | ||
i++ | ||
} | ||
} else { | ||
// Keep all of the different entries. | ||
for i < len(*jobs) && (*jobs)[i].Name == job.Name { | ||
shorterJobs = append(shorterJobs, (*jobs)[i]) | ||
} | ||
} | ||
} | ||
*jobs = shorterJobs | ||
} | ||
|
||
func sameSettings(jobs []presubmitJob, ref presubmitJob) bool { | ||
for _, job := range jobs { | ||
if job.Name != ref.Name { | ||
continue | ||
} | ||
if job.RunIfChanged != ref.RunIfChanged || | ||
job.SkipIfOnlyChanged != ref.SkipIfOnlyChanged { | ||
return false | ||
} | ||
} | ||
return true | ||
} | ||
|
||
// maybeAbsPath tries to make a path absolute. This is useful because | ||
// relative paths in test output tend to be confusing when the user | ||
// invoked the test outside of the test's directory. | ||
func maybeAbsPath(path string) string { | ||
if path, err := filepath.Abs(path); err == nil { | ||
return path | ||
} | ||
return path | ||
} |
Oops, something went wrong.