Skip to content

Commit

Permalink
tests: ensure that pre-submits get additional reviews
Browse files Browse the repository at this point in the history
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
pohly committed Dec 17, 2024
1 parent 88405ac commit aa8ef28
Show file tree
Hide file tree
Showing 8 changed files with 407 additions and 3 deletions.
7 changes: 7 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ update-go-deps:
.PHONY: verify-go-deps
verify-go-deps:
hack/make-rules/verify/go-deps.sh
.PHONY: update-unit
.PHONY: update-go-unit
update-unit: update-go-unit
UPDATE_FIXTURE_DATA=true hack/make-rules/go-test/unit.sh
.PHONY: update-config-fixture
update-config-fixture:
UPDATE_FIXTURE_DATA=true hack/make-rules/go-test/unit.sh ./config/tests/...
# ======================== Image Building/Publishing ===========================
# Build and publish miscellaneous images that get pushed to the specified REGISTRY.
# The full set of images covered by these targets is configured in
Expand Down
3 changes: 3 additions & 0 deletions config/tests/jobs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
14 changes: 14 additions & 0 deletions config/tests/jobs/policy/OWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# See the OWNERS docs at https://go.k8s.io/owners

reviewers:
- test-infra-oncall # oncall
- BenTheElder # lead
- aojea # lead
approvers:
- test-infra-oncall # oncall
- BenTheElder # lead
- aojea # lead

labels:
- sig/testing
- sig/infra
13 changes: 13 additions & 0 deletions config/tests/jobs/policy/README.md
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.
257 changes: 257 additions & 0 deletions config/tests/jobs/policy/policy_test.go
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
}
Loading

0 comments on commit aa8ef28

Please sign in to comment.