Skip to content

Commit 42d46db

Browse files
committed
tests: ensure that pre-submits get additional reviews
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 kubernetes/community#8196. To ensure that this is considered when defining pre-submit jobs, they need to be listed in `config/tests/jobs/presubmit-jobs.yaml`. UPDATE_FIXTURE_DATA=true re-generates that file to the expected state for inclusion in a PR.
1 parent 63168f3 commit 42d46db

File tree

5 files changed

+344
-3
lines changed

5 files changed

+344
-3
lines changed

config/tests/jobs/README.md

Lines changed: 3 additions & 0 deletions
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

config/tests/jobs/jobs_test.go

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,13 @@ import (
3535
"testing"
3636
"time"
3737

38+
"github.com/google/go-cmp/cmp"
39+
"github.com/google/go-cmp/cmp/cmpopts"
3840
coreapi "k8s.io/api/core/v1"
3941
"k8s.io/apimachinery/pkg/api/resource"
4042
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
4143
"k8s.io/apimachinery/pkg/util/sets"
44+
yaml "sigs.k8s.io/yaml/goyaml.v3"
4245

4346
prowapi "sigs.k8s.io/prow/pkg/apis/prowjobs/v1"
4447
cfg "sigs.k8s.io/prow/pkg/config"
@@ -1393,3 +1396,180 @@ func TestKubernetesProwJobsShouldNotUseDeprecatedScenarios(t *testing.T) {
13931396
t.Logf("summary: %v/%v jobs using deprecated scenarios", jobsToFix, len(jobs))
13941397
}
13951398
}
1399+
1400+
func TestKubernetesPresubmitJobs(t *testing.T) {
1401+
jobs := c.AllStaticPresubmits([]string{"kubernetes/kubernetes"})
1402+
var expected presubmitJobs
1403+
1404+
for _, job := range jobs {
1405+
if !job.AlwaysRun && job.RunIfChanged == "" {
1406+
// Manually triggered, no additional review needed.
1407+
continue
1408+
}
1409+
1410+
// Mirror those attributes of the job which must trigger additional reviews
1411+
// or are needed to identify the job.
1412+
j := presubmitJob{
1413+
Name: job.Name,
1414+
SkipBranches: job.SkipBranches,
1415+
Branches: job.Branches,
1416+
1417+
Optional: job.Optional,
1418+
RunIfChanged: job.RunIfChanged,
1419+
SkipIfOnlyChanged: job.SkipIfOnlyChanged,
1420+
}
1421+
1422+
// This uses separate top-level fields instead of job attributes to
1423+
// make it more obvious when run_if_changed is used.
1424+
if job.AlwaysRun {
1425+
expected.AlwaysRun = append(expected.AlwaysRun, j)
1426+
} else {
1427+
expected.RunIfChanged = append(expected.RunIfChanged, j)
1428+
}
1429+
1430+
}
1431+
expected.Normalize()
1432+
1433+
// Encode the expected content.
1434+
var expectedData bytes.Buffer
1435+
if _, err := expectedData.Write([]byte(`# AUTOGENERATED by "UPDATE_FIXTURE_DATA=true go test ./config/tests/jobs". DO NOT EDIT!
1436+
1437+
`)); err != nil {
1438+
t.Fatalf("unexpected error writing into buffer: %v", err)
1439+
}
1440+
1441+
encoder := yaml.NewEncoder(&expectedData)
1442+
encoder.SetIndent(4)
1443+
if err := encoder.Encode(expected); err != nil {
1444+
t.Fatalf("unexpected error encoding %s: %v", presubmitsFile, err)
1445+
}
1446+
1447+
// Compare. This proceeds on read or decoding errors because
1448+
// the file might get re-generated below.
1449+
var actual presubmitJobs
1450+
actualData, err := os.ReadFile(presubmitsFile)
1451+
if err != nil && !os.IsNotExist(err) {
1452+
t.Errorf("unexpected error: %v", err)
1453+
}
1454+
if err := yaml.Unmarshal(actualData, &actual); err != nil {
1455+
t.Errorf("unexpected error decoding %s: %v", presubmitsFile, err)
1456+
}
1457+
1458+
// First check the in-memory structs. The diff is nicer for them (more context).
1459+
diff := cmp.Diff(actual, expected)
1460+
if diff == "" {
1461+
// Next check the encoded data. This should only be different on test updates.
1462+
diff = cmp.Diff(string(actualData), expectedData.String(), cmpopts.AcyclicTransformer("SplitLines", func(s string) []string {
1463+
return strings.Split(s, "\n")
1464+
}))
1465+
}
1466+
1467+
if diff != "" {
1468+
t.Errorf(`
1469+
%s is out-dated. Detected differences (- actual, + expected):
1470+
%s
1471+
1472+
Blocking pre-submit jobs must be for stable, important features.
1473+
Non-blocking pre-submit jobs should only be run automatically if they meet
1474+
the criteria outlined in https://github.com/kubernetes/community/pull/8196.
1475+
1476+
To ensure that this is considered when defining pre-submit jobs, they
1477+
need to be listed in %s. If the pre-submit job is really needed,
1478+
re-run the test with UPDATE_FIXTURE_DATA=true and include the modified
1479+
file.
1480+
1481+
1482+
`, presubmitsFile, diff, presubmitsFile)
1483+
if value, _ := os.LookupEnv("UPDATE_FIXTURE_DATA"); value == "true" {
1484+
if err := os.WriteFile(presubmitsFile, expectedData.Bytes(), 0644); err != nil {
1485+
t.Fatalf("unexpected error: %v", err)
1486+
}
1487+
}
1488+
}
1489+
}
1490+
1491+
// presubmitsFile contains the following struct.
1492+
const presubmitsFile = "presubmit-jobs.yaml"
1493+
1494+
type presubmitJobs struct {
1495+
AlwaysRun []presubmitJob `yaml:"always_run"`
1496+
RunIfChanged []presubmitJob `yaml:"run_if_changed"`
1497+
}
1498+
type presubmitJob struct {
1499+
Name string `yaml:"name"`
1500+
SkipBranches []string `yaml:"skip_branches,omitempty"`
1501+
Branches []string `yaml:"branches,omitempty"`
1502+
Optional bool `yaml:"optional,omitempty"`
1503+
RunIfChanged string `yaml:"run_if_changed,omitempty"`
1504+
SkipIfOnlyChanged string `yaml:"skip_if_only_changed,omitempty"`
1505+
}
1506+
1507+
func (p *presubmitJobs) Normalize() {
1508+
sortJobs(&p.AlwaysRun)
1509+
sortJobs(&p.RunIfChanged)
1510+
}
1511+
1512+
func sortJobs(jobs *[]presubmitJob) {
1513+
for _, job := range *jobs {
1514+
sort.Strings(job.SkipBranches)
1515+
sort.Strings(job.Branches)
1516+
}
1517+
sort.Slice(*jobs, func(i, j int) bool {
1518+
switch strings.Compare((*jobs)[i].Name, (*jobs)[j].Name) {
1519+
case -1:
1520+
return true
1521+
case 1:
1522+
return false
1523+
}
1524+
switch slices.Compare((*jobs)[i].SkipBranches, (*jobs)[j].SkipBranches) {
1525+
case -1:
1526+
return true
1527+
case 1:
1528+
return false
1529+
}
1530+
switch slices.Compare((*jobs)[i].Branches, (*jobs)[j].Branches) {
1531+
case -1:
1532+
return true
1533+
case 1:
1534+
return false
1535+
}
1536+
return false
1537+
})
1538+
1539+
// If a job has the same settings regardless of the branch, then
1540+
// we can reduce to a single entry without the branch info.
1541+
shorterJobs := make([]presubmitJob, 0, len(*jobs))
1542+
for i := 0; i < len(*jobs); {
1543+
job := (*jobs)[i]
1544+
job.Branches = nil
1545+
job.SkipBranches = nil
1546+
1547+
if sameSettings(*jobs, job) {
1548+
shorterJobs = append(shorterJobs, job)
1549+
// Fast-forward to next job.
1550+
for i < len(*jobs) && (*jobs)[i].Name == job.Name {
1551+
i++
1552+
}
1553+
} else {
1554+
// Keep all of the different entries.
1555+
for i < len(*jobs) && (*jobs)[i].Name == job.Name {
1556+
shorterJobs = append(shorterJobs, (*jobs)[i])
1557+
}
1558+
}
1559+
}
1560+
*jobs = shorterJobs
1561+
}
1562+
1563+
func sameSettings(jobs []presubmitJob, ref presubmitJob) bool {
1564+
for _, job := range jobs {
1565+
if job.Name != ref.Name {
1566+
continue
1567+
}
1568+
if job.Optional != ref.Optional ||
1569+
job.RunIfChanged != ref.RunIfChanged ||
1570+
job.SkipIfOnlyChanged != ref.SkipIfOnlyChanged {
1571+
return false
1572+
}
1573+
}
1574+
return true
1575+
}

config/tests/jobs/presubmit-jobs.yaml

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
# AUTOGENERATED by "UPDATE_FIXTURE_DATA=true go test ./config/tests/jobs". DO NOT EDIT!
2+
3+
always_run:
4+
- name: pull-kubernetes-conformance-kind-ga-only-parallel
5+
- name: pull-kubernetes-dependencies
6+
- name: pull-kubernetes-e2e-ec2
7+
optional: true
8+
- name: pull-kubernetes-e2e-ec2-conformance
9+
optional: true
10+
- name: pull-kubernetes-e2e-gce
11+
- name: pull-kubernetes-e2e-gce-canary
12+
optional: true
13+
- name: pull-kubernetes-e2e-kind
14+
- name: pull-kubernetes-e2e-kind-ipv6
15+
- name: pull-kubernetes-integration
16+
- name: pull-kubernetes-integration-go-compatibility
17+
- name: pull-kubernetes-linter-hints
18+
optional: true
19+
- name: pull-kubernetes-node-e2e-containerd
20+
- name: pull-kubernetes-typecheck
21+
- name: pull-kubernetes-unit
22+
- name: pull-kubernetes-unit-go-compatibility
23+
- name: pull-kubernetes-verify
24+
run_if_changed:
25+
- name: check-dependency-stats
26+
optional: true
27+
run_if_changed: ^(go.mod|go.sum|vendor)
28+
- name: pull-kubernetes-apidiff-client-go
29+
optional: true
30+
run_if_changed: ((^staging\/src\/k8s.io\/client-go)|(^staging\/src\/k8s.io\/code-generator\/examples))/.*\.go
31+
- name: pull-kubernetes-conformance-image-test
32+
optional: true
33+
run_if_changed: conformance
34+
- name: pull-kubernetes-conformance-kind-ipv6-parallel
35+
optional: true
36+
run_if_changed: ^test/
37+
- name: pull-kubernetes-cross
38+
optional: true
39+
run_if_changed: (^.go-version)|(^build/build-image/)|(^hack/lib/golang.sh)|(^build/common.sh)
40+
- name: pull-kubernetes-e2e-autoscaling-hpa-cm
41+
optional: true
42+
run_if_changed: ^(pkg\/controller\/podautoscaler\/|test\/e2e\/autoscaling\/custom_metrics_stackdriver_autoscaling.go$)
43+
- name: pull-kubernetes-e2e-autoscaling-hpa-cpu
44+
optional: true
45+
run_if_changed: ^(pkg\/controller\/podautoscaler\/|test\/e2e\/autoscaling\/horizontal_pod_autoscaling|test\/e2e\/framework\/autoscaling\/)
46+
- name: pull-kubernetes-e2e-capz-azure-disk
47+
optional: true
48+
run_if_changed: azure.*\.go
49+
- name: pull-kubernetes-e2e-capz-azure-disk-vmss
50+
optional: true
51+
run_if_changed: azure.*\.go
52+
- name: pull-kubernetes-e2e-capz-azure-disk-windows
53+
optional: true
54+
run_if_changed: azure.*\.go
55+
- name: pull-kubernetes-e2e-capz-azure-file
56+
optional: true
57+
run_if_changed: azure.*\.go
58+
- name: pull-kubernetes-e2e-capz-azure-file-vmss
59+
optional: true
60+
run_if_changed: azure.*\.go
61+
- name: pull-kubernetes-e2e-capz-azure-file-windows
62+
optional: true
63+
run_if_changed: azure.*\.go
64+
- name: pull-kubernetes-e2e-capz-conformance
65+
optional: true
66+
run_if_changed: azure.*\.go
67+
- name: pull-kubernetes-e2e-capz-windows-1-29
68+
optional: true
69+
run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.*
70+
- name: pull-kubernetes-e2e-capz-windows-1-30
71+
optional: true
72+
run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.*
73+
- name: pull-kubernetes-e2e-capz-windows-1-31
74+
optional: true
75+
run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.*
76+
- name: pull-kubernetes-e2e-capz-windows-1-32
77+
optional: true
78+
run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.*
79+
- name: pull-kubernetes-e2e-capz-windows-master
80+
optional: true
81+
run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.*
82+
- name: pull-kubernetes-e2e-gce-cos-alpha-features
83+
optional: true
84+
run_if_changed: ^.*feature.*\.go
85+
- name: pull-kubernetes-e2e-gce-csi-serial
86+
optional: true
87+
run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi)
88+
- name: pull-kubernetes-e2e-gce-kubelet-credential-provider
89+
optional: true
90+
run_if_changed: ^(pkg\/credentialprovider\/|test\/e2e_node\/plugins\/gcp-credential-provider)
91+
- name: pull-kubernetes-e2e-gce-network-policies
92+
optional: true
93+
run_if_changed: ^(test/e2e/network/|pkg/apis/networking)
94+
- name: pull-kubernetes-e2e-gce-network-proxy-grpc
95+
optional: true
96+
run_if_changed: ^(cluster/gce/manifests/konnectivity-server.yaml$|cluster/gce/addons/konnectivity-agent)
97+
- name: pull-kubernetes-e2e-gce-network-proxy-http-connect
98+
run_if_changed: ^(cluster/gce/manifests/konnectivity-server.yaml$|cluster/gce/addons/konnectivity-agent)
99+
- name: pull-kubernetes-e2e-gce-providerless-1-30
100+
optional: true
101+
run_if_changed: (provider|cloud-controller-manager|cloud|ipam|azure|legacy-cloud-providers|test\/e2e\/cloud\/gcp|test\/e2e\/framework\/provider|nvidia|accelerator|test\/e2e\/network|test\/e2e\/storage)
102+
- name: pull-kubernetes-e2e-gce-storage-slow
103+
optional: true
104+
run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi)
105+
- name: pull-kubernetes-e2e-gce-storage-snapshot
106+
optional: true
107+
run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi)
108+
- name: pull-kubernetes-e2e-gci-gce-autoscaling
109+
optional: true
110+
run_if_changed: ^(cluster/gce/manifests/cluster-autoscaler.manifest$|cluster/addons/rbac/cluster-autoscaler)
111+
- name: pull-kubernetes-e2e-gci-gce-ingress
112+
optional: true
113+
run_if_changed: ^(test/e2e/network/|pkg/apis/networking)
114+
- name: pull-kubernetes-e2e-gci-gce-ipvs
115+
optional: true
116+
run_if_changed: ^(test/e2e/network/|pkg/apis/networking|pkg/.*/ipvs/)
117+
- name: pull-kubernetes-e2e-inplace-pod-resize-containerd-main-v2
118+
optional: true
119+
run_if_changed: test/e2e/node/pod_resize.go|pkg/kubelet/kubelet.go|pkg/kubelet/kubelet_pods.go|pkg/kubelet/kuberuntime/kuberuntime_manager.go
120+
- name: pull-kubernetes-e2e-kind-alpha-beta-features
121+
optional: true
122+
run_if_changed: ^pkg/features/
123+
- name: pull-kubernetes-e2e-kind-ipvs
124+
optional: true
125+
run_if_changed: ^(test/e2e/network/|pkg/apis/networking|pkg/proxy/ipvs/)
126+
- name: pull-kubernetes-e2e-kind-kms
127+
optional: true
128+
run_if_changed: staging/src/k8s.io/apiserver/pkg/storage/value/|staging/src/k8s.io/kms/|staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/|test/e2e/testing-manifests/auth/encrypt/
129+
- name: pull-kubernetes-e2e-kind-nftables
130+
optional: true
131+
run_if_changed: ^(test/e2e/network/|pkg/apis/networking|pkg/proxy/nftables/)
132+
- name: pull-kubernetes-e2e-storage-kind-disruptive
133+
optional: true
134+
run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi)
135+
- name: pull-kubernetes-kind-dra
136+
optional: true
137+
run_if_changed: /(dra|dynamicresources|resourceclaim|deviceclass|resourceslice|resourceclaimtemplate|dynamic-resource-allocation|pkg/apis/resource|api/resource)/.*.go
138+
- name: pull-kubernetes-kind-dra-all
139+
optional: true
140+
run_if_changed: /(dra|dynamicresources|resourceclaim|deviceclass|resourceslice|resourceclaimtemplate|dynamic-resource-allocation|pkg/apis/resource|api/resource)/.*.go
141+
- name: pull-kubernetes-kind-json-logging
142+
optional: true
143+
run_if_changed: /staging/src/k8s.io/component-base/logs/
144+
- name: pull-kubernetes-kind-text-logging
145+
optional: true
146+
run_if_changed: /staging/src/k8s.io/component-base/logs/
147+
- name: pull-kubernetes-local-e2e
148+
optional: true
149+
run_if_changed: local-up-cluster
150+
- name: pull-kubernetes-node-e2e-crio-cgrpv1-dra
151+
optional: true
152+
run_if_changed: (/dra/|/dynamicresources/|/resourceclaim/|/deviceclass/|/resourceslice/|/resourceclaimtemplate/|/dynamic-resource-allocation/|/pkg/apis/resource/|/api/resource/|/test/e2e_node/dra_).*\.(go|yaml)
153+
- name: pull-kubernetes-node-kubelet-credential-provider
154+
optional: true
155+
run_if_changed: ^(pkg\/credentialprovider\/|test\/e2e_node\/plugins\/gcp-credential-provider)
156+
- name: pull-publishing-bot-validate
157+
optional: true
158+
run_if_changed: ^staging/publishing.*$

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ require (
8787
sigs.k8s.io/controller-runtime v0.12.3
8888
sigs.k8s.io/controller-tools v0.9.2
8989
sigs.k8s.io/prow v0.0.0-20240419142743-3cb2506c2ff3
90-
sigs.k8s.io/yaml v1.3.0
90+
sigs.k8s.io/yaml v1.4.0
9191
)
9292

9393
require (

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1158,5 +1158,5 @@ sigs.k8s.io/structured-merge-diff/v4 v4.2.3 h1:PRbqxJClWWYMNV1dhaG4NsibJbArud9kF
11581158
sigs.k8s.io/structured-merge-diff/v4 v4.2.3/go.mod h1:qjx8mGObPmV2aSZepjQjbmb2ihdVs8cGKBraizNC69E=
11591159
sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o=
11601160
sigs.k8s.io/yaml v1.2.0/go.mod h1:yfXDCHCao9+ENCvLSE62v9VSji2MKu5jeNfTrofGhJc=
1161-
sigs.k8s.io/yaml v1.3.0 h1:a2VclLzOGrwOHDiV8EfBGhvjHvP46CtW5j6POvhYGGo=
1162-
sigs.k8s.io/yaml v1.3.0/go.mod h1:GeOyir5tyXNByN85N/dRIT9es5UQNerPYEKK56eTBm8=
1161+
sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E=
1162+
sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY=

0 commit comments

Comments
 (0)