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.

config/tests/jobs/README.md

@@ -8,4 +8,7 @@ To run via bazel: `bazel test //config/tests/jobs/...`
 
 To run via go: `go test .`
 
+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.
+
 [prow.k8s.io]: https://prow.k8s.io

config/tests/jobs/jobs_test.go

@@ -35,10 +35,13 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	coreapi "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
+	yaml "sigs.k8s.io/yaml/goyaml.v3"
 
 	prowapi "sigs.k8s.io/prow/pkg/apis/prowjobs/v1"
 	cfg "sigs.k8s.io/prow/pkg/config"
@@ -1393,3 +1396,180 @@ func TestKubernetesProwJobsShouldNotUseDeprecatedScenarios(t *testing.T) {
 		t.Logf("summary: %v/%v jobs using deprecated scenarios", jobsToFix, len(jobs))
 	}
 }
+
+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,
+
+			Optional:          job.Optional,
+			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)
+		}
+
+	}
+	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 != "" {
+		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.
+
+
+`, presubmitsFile, diff, presubmitsFile)
+		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)
+			}
+		}
+	}
+}
+
+// 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"`
+	Optional          bool     `yaml:"optional,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.Optional != ref.Optional ||
+			job.RunIfChanged != ref.RunIfChanged ||
+			job.SkipIfOnlyChanged != ref.SkipIfOnlyChanged {
+			return false
+		}
+	}
+	return true
+}

config/tests/jobs/presubmit-jobs.yaml

@@ -0,0 +1,158 @@
+# AUTOGENERATED by "UPDATE_FIXTURE_DATA=true go test ./config/tests/jobs". DO NOT EDIT!
+
+always_run:
+    - name: pull-kubernetes-conformance-kind-ga-only-parallel
+    - name: pull-kubernetes-dependencies
+    - name: pull-kubernetes-e2e-ec2
+      optional: true
+    - name: pull-kubernetes-e2e-ec2-conformance
+      optional: true
+    - name: pull-kubernetes-e2e-gce
+    - name: pull-kubernetes-e2e-gce-canary
+      optional: true
+    - name: pull-kubernetes-e2e-kind
+    - name: pull-kubernetes-e2e-kind-ipv6
+    - name: pull-kubernetes-integration
+    - name: pull-kubernetes-integration-go-compatibility
+    - name: pull-kubernetes-linter-hints
+      optional: true
+    - name: pull-kubernetes-node-e2e-containerd
+    - name: pull-kubernetes-typecheck
+    - name: pull-kubernetes-unit
+    - name: pull-kubernetes-unit-go-compatibility
+    - name: pull-kubernetes-verify
+run_if_changed:
+    - name: check-dependency-stats
+      optional: true
+      run_if_changed: ^(go.mod|go.sum|vendor)
+    - name: pull-kubernetes-apidiff-client-go
+      optional: true
+      run_if_changed: ((^staging\/src\/k8s.io\/client-go)|(^staging\/src\/k8s.io\/code-generator\/examples))/.*\.go
+    - name: pull-kubernetes-conformance-image-test
+      optional: true
+      run_if_changed: conformance
+    - name: pull-kubernetes-conformance-kind-ipv6-parallel
+      optional: true
+      run_if_changed: ^test/
+    - name: pull-kubernetes-cross
+      optional: true
+      run_if_changed: (^.go-version)|(^build/build-image/)|(^hack/lib/golang.sh)|(^build/common.sh)
+    - name: pull-kubernetes-e2e-autoscaling-hpa-cm
+      optional: true
+      run_if_changed: ^(pkg\/controller\/podautoscaler\/|test\/e2e\/autoscaling\/custom_metrics_stackdriver_autoscaling.go$)
+    - name: pull-kubernetes-e2e-autoscaling-hpa-cpu
+      optional: true
+      run_if_changed: ^(pkg\/controller\/podautoscaler\/|test\/e2e\/autoscaling\/horizontal_pod_autoscaling|test\/e2e\/framework\/autoscaling\/)
+    - name: pull-kubernetes-e2e-capz-azure-disk
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-azure-disk-vmss
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-azure-disk-windows
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-azure-file
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-azure-file-vmss
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-azure-file-windows
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-conformance
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-windows-1-29
+      optional: true
+      run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.*
+    - name: pull-kubernetes-e2e-capz-windows-1-30
+      optional: true
+      run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.*
+    - name: pull-kubernetes-e2e-capz-windows-1-31
+      optional: true
+      run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.*
+    - name: pull-kubernetes-e2e-capz-windows-1-32
+      optional: true
+      run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.*
+    - name: pull-kubernetes-e2e-capz-windows-master
+      optional: true
+      run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.*
+    - name: pull-kubernetes-e2e-gce-cos-alpha-features
+      optional: true
+      run_if_changed: ^.*feature.*\.go
+    - name: pull-kubernetes-e2e-gce-csi-serial
+      optional: true
+      run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi)
+    - name: pull-kubernetes-e2e-gce-kubelet-credential-provider
+      optional: true
+      run_if_changed: ^(pkg\/credentialprovider\/|test\/e2e_node\/plugins\/gcp-credential-provider)
+    - name: pull-kubernetes-e2e-gce-network-policies
+      optional: true
+      run_if_changed: ^(test/e2e/network/|pkg/apis/networking)
+    - name: pull-kubernetes-e2e-gce-network-proxy-grpc
+      optional: true
+      run_if_changed: ^(cluster/gce/manifests/konnectivity-server.yaml$|cluster/gce/addons/konnectivity-agent)
+    - name: pull-kubernetes-e2e-gce-network-proxy-http-connect
+      run_if_changed: ^(cluster/gce/manifests/konnectivity-server.yaml$|cluster/gce/addons/konnectivity-agent)
+    - name: pull-kubernetes-e2e-gce-providerless-1-30
+      optional: true
+      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)
+    - name: pull-kubernetes-e2e-gce-storage-slow
+      optional: true
+      run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi)
+    - name: pull-kubernetes-e2e-gce-storage-snapshot
+      optional: true
+      run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi)
+    - name: pull-kubernetes-e2e-gci-gce-autoscaling
+      optional: true
+      run_if_changed: ^(cluster/gce/manifests/cluster-autoscaler.manifest$|cluster/addons/rbac/cluster-autoscaler)
+    - name: pull-kubernetes-e2e-gci-gce-ingress
+      optional: true
+      run_if_changed: ^(test/e2e/network/|pkg/apis/networking)
+    - name: pull-kubernetes-e2e-gci-gce-ipvs
+      optional: true
+      run_if_changed: ^(test/e2e/network/|pkg/apis/networking|pkg/.*/ipvs/)
+    - name: pull-kubernetes-e2e-inplace-pod-resize-containerd-main-v2
+      optional: true
+      run_if_changed: test/e2e/node/pod_resize.go|pkg/kubelet/kubelet.go|pkg/kubelet/kubelet_pods.go|pkg/kubelet/kuberuntime/kuberuntime_manager.go
+    - name: pull-kubernetes-e2e-kind-alpha-beta-features
+      optional: true
+      run_if_changed: ^pkg/features/
+    - name: pull-kubernetes-e2e-kind-ipvs
+      optional: true
+      run_if_changed: ^(test/e2e/network/|pkg/apis/networking|pkg/proxy/ipvs/)
+    - name: pull-kubernetes-e2e-kind-kms
+      optional: true
+      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/
+    - name: pull-kubernetes-e2e-kind-nftables
+      optional: true
+      run_if_changed: ^(test/e2e/network/|pkg/apis/networking|pkg/proxy/nftables/)
+    - name: pull-kubernetes-e2e-storage-kind-disruptive
+      optional: true
+      run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi)
+    - name: pull-kubernetes-kind-dra
+      optional: true
+      run_if_changed: /(dra|dynamicresources|resourceclaim|deviceclass|resourceslice|resourceclaimtemplate|dynamic-resource-allocation|pkg/apis/resource|api/resource)/.*.go
+    - name: pull-kubernetes-kind-dra-all
+      optional: true
+      run_if_changed: /(dra|dynamicresources|resourceclaim|deviceclass|resourceslice|resourceclaimtemplate|dynamic-resource-allocation|pkg/apis/resource|api/resource)/.*.go
+    - name: pull-kubernetes-kind-json-logging
+      optional: true
+      run_if_changed: /staging/src/k8s.io/component-base/logs/
+    - name: pull-kubernetes-kind-text-logging
+      optional: true
+      run_if_changed: /staging/src/k8s.io/component-base/logs/
+    - name: pull-kubernetes-local-e2e
+      optional: true
+      run_if_changed: local-up-cluster
+    - name: pull-kubernetes-node-e2e-crio-cgrpv1-dra
+      optional: true
+      run_if_changed: (/dra/|/dynamicresources/|/resourceclaim/|/deviceclass/|/resourceslice/|/resourceclaimtemplate/|/dynamic-resource-allocation/|/pkg/apis/resource/|/api/resource/|/test/e2e_node/dra_).*\.(go|yaml)
+    - name: pull-kubernetes-node-kubelet-credential-provider
+      optional: true
+      run_if_changed: ^(pkg\/credentialprovider\/|test\/e2e_node\/plugins\/gcp-credential-provider)
+    - name: pull-publishing-bot-validate
+      optional: true
+      run_if_changed: ^staging/publishing.*$

go.mod

@@ -87,7 +87,7 @@ require (
 	sigs.k8s.io/controller-runtime v0.12.3
 	sigs.k8s.io/controller-tools v0.9.2
 	sigs.k8s.io/prow v0.0.0-20240419142743-3cb2506c2ff3
-	sigs.k8s.io/yaml v1.3.0
+	sigs.k8s.io/yaml v1.4.0
 )
 
 require (

go.sum

@@ -1158,5 +1158,5 @@ sigs.k8s.io/structured-merge-diff/v4 v4.2.3 h1:PRbqxJClWWYMNV1dhaG4NsibJbArud9kF
 sigs.k8s.io/structured-merge-diff/v4 v4.2.3/go.mod h1:qjx8mGObPmV2aSZepjQjbmb2ihdVs8cGKBraizNC69E=
 sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o=
 sigs.k8s.io/yaml v1.2.0/go.mod h1:yfXDCHCao9+ENCvLSE62v9VSji2MKu5jeNfTrofGhJc=
-sigs.k8s.io/yaml v1.3.0 h1:a2VclLzOGrwOHDiV8EfBGhvjHvP46CtW5j6POvhYGGo=
-sigs.k8s.io/yaml v1.3.0/go.mod h1:GeOyir5tyXNByN85N/dRIT9es5UQNerPYEKK56eTBm8=
+sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E=
+sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY=