Skip to content

Commit aa8ef28

Browse files
committed
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.
1 parent 88405ac commit aa8ef28

File tree

8 files changed

+407
-3
lines changed

8 files changed

+407
-3
lines changed

Diff for: Makefile

+7
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,13 @@ update-go-deps:
5959
.PHONY: verify-go-deps
6060
verify-go-deps:
6161
hack/make-rules/verify/go-deps.sh
62+
.PHONY: update-unit
63+
.PHONY: update-go-unit
64+
update-unit: update-go-unit
65+
UPDATE_FIXTURE_DATA=true hack/make-rules/go-test/unit.sh
66+
.PHONY: update-config-fixture
67+
update-config-fixture:
68+
UPDATE_FIXTURE_DATA=true hack/make-rules/go-test/unit.sh ./config/tests/...
6269
# ======================== Image Building/Publishing ===========================
6370
# Build and publish miscellaneous images that get pushed to the specified REGISTRY.
6471
# The full set of images covered by these targets is configured in

Diff for: config/tests/jobs/README.md

+3
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,7 @@ To run via bazel: `bazel test //config/tests/jobs/...`
88

99
To run via go: `go test .`
1010

11+
If tests fail, re-run with the `UPDATE_FIXTURE_DATA=true` env variable
12+
and include the modified files in the PR which updates the jobs.
13+
1114
[prow.k8s.io]: https://prow.k8s.io

Diff for: config/tests/jobs/policy/OWNERS

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# See the OWNERS docs at https://go.k8s.io/owners
2+
3+
reviewers:
4+
- test-infra-oncall # oncall
5+
- BenTheElder # lead
6+
- aojea # lead
7+
approvers:
8+
- test-infra-oncall # oncall
9+
- BenTheElder # lead
10+
- aojea # lead
11+
12+
labels:
13+
- sig/testing
14+
- sig/infra

Diff for: config/tests/jobs/policy/README.md

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
The project has certain guidelines around jobs which are meant to ensure that
2+
there's a balance between test coverage and costs for running the CI. For
3+
example, non-blocking jobs that get trigger automatically for PRs should be
4+
used judiciously.
5+
6+
Because SIG leads are not necessarily familiar with those policies, SIG Testing
7+
and SIG Infra need to be involved before merging jobs that fall into those
8+
sensitive areas. This is achieved with tests and additional files in this
9+
directory and a separate OWNERS file.
10+
11+
To check whether jobs are okay, run the Go tests in this directory.
12+
If tests fail, re-run with the `UPDATE_FIXTURE_DATA=true` env variable
13+
and include the modified files in the PR which updates the jobs.

Diff for: config/tests/jobs/policy/policy_test.go

+257
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,257 @@
1+
/*
2+
Copyright 2018 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package policy
18+
19+
// This file validates Kubernetes's jobs configs against policies.
20+
21+
import (
22+
"bytes"
23+
"flag"
24+
"fmt"
25+
"os"
26+
"path/filepath"
27+
"slices"
28+
"sort"
29+
"strings"
30+
"testing"
31+
32+
"github.com/google/go-cmp/cmp"
33+
"github.com/google/go-cmp/cmp/cmpopts"
34+
yaml "sigs.k8s.io/yaml/goyaml.v3"
35+
36+
cfg "sigs.k8s.io/prow/pkg/config"
37+
)
38+
39+
var configPath = flag.String("config", "../../../../config/prow/config.yaml", "Path to prow config")
40+
var jobConfigPath = flag.String("job-config", "../../../jobs", "Path to prow job config")
41+
var deckPath = flag.String("deck-path", "https://prow.k8s.io", "Path to deck")
42+
var bucket = flag.String("bucket", "kubernetes-ci-logs", "Gcs bucket for log upload")
43+
var k8sProw = flag.Bool("k8s-prow", true, "If the config is for k8s prow cluster")
44+
45+
// Loaded at TestMain.
46+
var c *cfg.Config
47+
48+
func TestMain(m *testing.M) {
49+
flag.Parse()
50+
if *configPath == "" {
51+
fmt.Println("--config must set")
52+
os.Exit(1)
53+
}
54+
55+
conf, err := cfg.Load(*configPath, *jobConfigPath, nil, "")
56+
if err != nil {
57+
fmt.Printf("Could not load config: %v", err)
58+
os.Exit(1)
59+
}
60+
c = conf
61+
62+
os.Exit(m.Run())
63+
}
64+
65+
func TestKubernetesPresubmitJobs(t *testing.T) {
66+
jobs := c.AllStaticPresubmits([]string{"kubernetes/kubernetes"})
67+
var expected presubmitJobs
68+
69+
for _, job := range jobs {
70+
if !job.AlwaysRun && job.RunIfChanged == "" {
71+
// Manually triggered, no additional review needed.
72+
continue
73+
}
74+
75+
// Mirror those attributes of the job which must trigger additional reviews
76+
// or are needed to identify the job.
77+
j := presubmitJob{
78+
Name: job.Name,
79+
SkipBranches: job.SkipBranches,
80+
Branches: job.Branches,
81+
82+
RunIfChanged: job.RunIfChanged,
83+
SkipIfOnlyChanged: job.SkipIfOnlyChanged,
84+
}
85+
86+
// This uses separate top-level fields instead of job attributes to
87+
// make it more obvious when run_if_changed is used.
88+
if job.AlwaysRun {
89+
expected.AlwaysRun = append(expected.AlwaysRun, j)
90+
} else {
91+
expected.RunIfChanged = append(expected.RunIfChanged, j)
92+
93+
if !job.Optional {
94+
// Absolute path is more user-friendly than ../../config/...
95+
t.Errorf("Policy violation: %s in %s should use `optional: true` or `alwaysRun: true`.", job.Name, maybeAbsPath(job.SourcePath))
96+
}
97+
}
98+
99+
}
100+
expected.Normalize()
101+
102+
// Encode the expected content.
103+
var expectedData bytes.Buffer
104+
if _, err := expectedData.Write([]byte(`# AUTOGENERATED by "UPDATE_FIXTURE_DATA=true go test ./config/tests/jobs". DO NOT EDIT!
105+
106+
`)); err != nil {
107+
t.Fatalf("unexpected error writing into buffer: %v", err)
108+
}
109+
110+
encoder := yaml.NewEncoder(&expectedData)
111+
encoder.SetIndent(4)
112+
if err := encoder.Encode(expected); err != nil {
113+
t.Fatalf("unexpected error encoding %s: %v", presubmitsFile, err)
114+
}
115+
116+
// Compare. This proceeds on read or decoding errors because
117+
// the file might get re-generated below.
118+
var actual presubmitJobs
119+
actualData, err := os.ReadFile(presubmitsFile)
120+
if err != nil && !os.IsNotExist(err) {
121+
t.Errorf("unexpected error: %v", err)
122+
}
123+
if err := yaml.Unmarshal(actualData, &actual); err != nil {
124+
t.Errorf("unexpected error decoding %s: %v", presubmitsFile, err)
125+
}
126+
127+
// First check the in-memory structs. The diff is nicer for them (more context).
128+
diff := cmp.Diff(actual, expected)
129+
if diff == "" {
130+
// Next check the encoded data. This should only be different on test updates.
131+
diff = cmp.Diff(string(actualData), expectedData.String(), cmpopts.AcyclicTransformer("SplitLines", func(s string) []string {
132+
return strings.Split(s, "\n")
133+
}))
134+
}
135+
136+
if diff != "" {
137+
if value, _ := os.LookupEnv("UPDATE_FIXTURE_DATA"); value == "true" {
138+
if err := os.WriteFile(presubmitsFile, expectedData.Bytes(), 0644); err != nil {
139+
t.Fatalf("unexpected error: %v", err)
140+
}
141+
t.Logf(`
142+
%s was out-dated. Updated as requested with the following changes (- actual, + expected):
143+
%s
144+
`, maybeAbsPath(presubmitsFile), diff)
145+
} else {
146+
t.Errorf(`
147+
%s is out-dated. Detected differences (- actual, + expected):
148+
%s
149+
150+
Blocking pre-submit jobs must be for stable, important features.
151+
Non-blocking pre-submit jobs should only be run automatically if they meet
152+
the criteria outlined in https://github.com/kubernetes/community/pull/8196.
153+
154+
To ensure that this is considered when defining pre-submit jobs, they
155+
need to be listed in %s. If the pre-submit job is really needed,
156+
re-run the test with UPDATE_FIXTURE_DATA=true and include the modified
157+
file. The following command can be used:
158+
159+
make update-config-fixture
160+
`, presubmitsFile, diff, presubmitsFile)
161+
}
162+
}
163+
}
164+
165+
// presubmitsFile contains the following struct.
166+
const presubmitsFile = "presubmit-jobs.yaml"
167+
168+
type presubmitJobs struct {
169+
AlwaysRun []presubmitJob `yaml:"always_run"`
170+
RunIfChanged []presubmitJob `yaml:"run_if_changed"`
171+
}
172+
type presubmitJob struct {
173+
Name string `yaml:"name"`
174+
SkipBranches []string `yaml:"skip_branches,omitempty"`
175+
Branches []string `yaml:"branches,omitempty"`
176+
RunIfChanged string `yaml:"run_if_changed,omitempty"`
177+
SkipIfOnlyChanged string `yaml:"skip_if_only_changed,omitempty"`
178+
}
179+
180+
func (p *presubmitJobs) Normalize() {
181+
sortJobs(&p.AlwaysRun)
182+
sortJobs(&p.RunIfChanged)
183+
}
184+
185+
func sortJobs(jobs *[]presubmitJob) {
186+
for _, job := range *jobs {
187+
sort.Strings(job.SkipBranches)
188+
sort.Strings(job.Branches)
189+
}
190+
sort.Slice(*jobs, func(i, j int) bool {
191+
switch strings.Compare((*jobs)[i].Name, (*jobs)[j].Name) {
192+
case -1:
193+
return true
194+
case 1:
195+
return false
196+
}
197+
switch slices.Compare((*jobs)[i].SkipBranches, (*jobs)[j].SkipBranches) {
198+
case -1:
199+
return true
200+
case 1:
201+
return false
202+
}
203+
switch slices.Compare((*jobs)[i].Branches, (*jobs)[j].Branches) {
204+
case -1:
205+
return true
206+
case 1:
207+
return false
208+
}
209+
return false
210+
})
211+
212+
// If a job has the same settings regardless of the branch, then
213+
// we can reduce to a single entry without the branch info.
214+
shorterJobs := make([]presubmitJob, 0, len(*jobs))
215+
for i := 0; i < len(*jobs); {
216+
job := (*jobs)[i]
217+
job.Branches = nil
218+
job.SkipBranches = nil
219+
220+
if sameSettings(*jobs, job) {
221+
shorterJobs = append(shorterJobs, job)
222+
// Fast-forward to next job.
223+
for i < len(*jobs) && (*jobs)[i].Name == job.Name {
224+
i++
225+
}
226+
} else {
227+
// Keep all of the different entries.
228+
for i < len(*jobs) && (*jobs)[i].Name == job.Name {
229+
shorterJobs = append(shorterJobs, (*jobs)[i])
230+
}
231+
}
232+
}
233+
*jobs = shorterJobs
234+
}
235+
236+
func sameSettings(jobs []presubmitJob, ref presubmitJob) bool {
237+
for _, job := range jobs {
238+
if job.Name != ref.Name {
239+
continue
240+
}
241+
if job.RunIfChanged != ref.RunIfChanged ||
242+
job.SkipIfOnlyChanged != ref.SkipIfOnlyChanged {
243+
return false
244+
}
245+
}
246+
return true
247+
}
248+
249+
// maybeAbsPath tries to make a path absolute. This is useful because
250+
// relative paths in test output tend to be confusing when the user
251+
// invoked the test outside of the test's directory.
252+
func maybeAbsPath(path string) string {
253+
if path, err := filepath.Abs(path); err == nil {
254+
return path
255+
}
256+
return path
257+
}

0 commit comments

Comments
 (0)