Make CI much more effective #4564

KnVerey · 2022-04-01T21:56:32Z

In working on the linter config for #4560, I ended up discovering that our CI configuration as a whole is seriously problematic. Most seriously, many tests are run multiple times in the same env, and many others are never run at all. The license detection check doesn't work at all, and the linter is only applying to a few modules. 😱

I made a spreadsheet to inventory the situation, and it is perhaps the best way to see the before and after I'm trying to achieve with this PR: Kustomize test job inventory

Highlights of problems fixed

go test: This was being run up to five times per commit for some modules, and never for others. Most never ran master because they were only in the Prow check. They now fall into two categories: (1) everything outside the plugin/ directory runs once in each GOOS (except Windows, a la Fix breaking api tests on windows #4001) via the Github Action, including on master; (2) the plugin/ directory continues to only be run by Prow. We could revisit this later, but it didn't "just work" to move them over, and I am out of time to work on this for now.
go fix/fmt/tidy/vet: Prow (via kyaml precommit) ran go fix/fmt/tidy/vet on kyaml, cmd/config and functions/examples/*, but did nothing with the results. Those commands do not exit non-zero when they do something, and the git-based check that supposedly used to validate the result has been commented out for two years. I "fixed" this by not running most of them anymore, for any module. go mod tidy specifically is still run by Prow though, because it always ran that separately for all modules.
go build/generate: Prow (via kyaml precommit) also ran these for a few modules. I fixed this by adding a task that attempts to build all modules with a build target defined. Generate is only run if the build depends on it. There are generators that haven't been run for a long time and break things if you do run them, so generate couldn't simply be added globally.
make license: this was similarly only run in select places was similarly ineffective, since CI did not include the -check flag--it would make the changes, exit zero, and tests would pass. I fixed this by creating a single license file and a script to apply it to the entire repo. The script has a check mode, which CI now uses.
make lint: this was similarly only run in select places, but at least it worked for those places. Now it runs for all modules, including plugins. There are no actual linter fixes in this PR because the "since rev" option is pinned to such a recent commit. We could remove it on a branch and work on some of the existing issues like I did in Update and re-enable linters #4560, but that's out of scope for this PR.

Other notes

The Makefile symlinks were generated programmatically like this:

hack/for-each-module.sh 'if [[ ! -f Makefile ]]; then BACKTRACKS=$(pwd | sed -e "s|.*kustomize/||" -e "s|[^/]*|..|g"); ln -s $BACKTRACKS/Makefile-modules.mk Makefile; fi'

k8s-ci-robot · 2022-04-01T21:56:33Z

@KnVerey: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-04-01T21:56:33Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2022-04-01T21:56:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [KnVerey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

KnVerey · 2022-04-04T23:39:25Z

/test all

KnVerey · 2022-04-05T00:18:55Z

/test kustomize-presubmit-master

KnVerey · 2022-04-05T00:53:03Z

/test kustomize-presubmit-master

KnVerey · 2022-04-05T01:34:12Z

Makefile


-### Begin kustomize plugin rules.


This entire section was just lifted as-is into Makefile-plugins.mk to make this file easier to navigate.

KnVerey · 2022-04-05T01:45:03Z

hack/testUnitKustomizePlugins.sh

  make $MYGOBIN/helmV3
-  make $MYGOBIN/helm


neither old version seems to be used by anything anymore, so I removed the targets for these

KnVerey · 2022-04-05T01:45:57Z

/cc @natasha41575

natasha41575

Thanks for working on this! Some small comments

Makefile

natasha41575 · 2022-04-05T02:08:45Z

Makefile

+	install-tools \
+	test-unit-kustomize-plugins \
+	test-go-mod \
+	build-non-plugin-all \


Are we intentionally excluding test-non-plugin-all here?

If so, we should probably make the other CI jobs "required", right now only the CLA check and the kustomize-presubmit-master jobs are listed as required.

Yes, I did that because it seemed silly to run them twice, and desirable to (eventually) run all test in all three envs, which Github Actions is set up to do. If you can think of a reason we'd want to run them in prow as well, happy to add it back.

If so, we should probably make the other CI jobs "required", right now only the CLA check and the kustomize-presubmit-master jobs are listed as required.

Really surprised by this. I think they should be required either way. I'll change that now.

If you can think of a reason we'd want to run them in prow as well, happy to add it back.

No need to add it back. As long as it's running under a required job, removing redundancy SGTM.

kubernetes/test-infra#25878

Makefile

cmd/depprobcheck/Makefile

.github/workflows/go.yml

Makefile

natasha41575 · 2022-04-05T16:30:57Z

Makefile


 .PHONY: test-unit-kustomize-plugins
 test-unit-kustomize-plugins:
 	./hack/testUnitKustomizePlugins.sh

 .PHONY: test-unit-kustomize-cli
 test-unit-kustomize-cli:
-	cd kustomize; go test ./...
+	cd kustomize; $(MAKE) test


It won't let me comment on it, but right below this is test-unit-kustomize-all, is there any reason to have both that and test-unit-all?

It's also unclear to me why test-unit-kustomize-all chooses to test api, cli, and plugins, but not the rest of our unit tests. I'd lean towards just removing it unless there's a reason we need to keep it.

Makefile

KnVerey · 2022-04-05T19:00:50Z

Makefile

@@ -125,8 +126,9 @@ lint: $(MYGOBIN)/golangci-lint $(MYGOBIN)/goimports $(builtinplugins)
 	./hack/for-each-module.sh "make lint"

 .PHONY: test-unit-all
-test-unit-all: $(builtinplugins)
-	./hack/for-each-module.sh "make test"


It would be nice to use this definition, but it doesn't work right now. The plugin script is doing some setup / exlusions.

natasha41575 · 2022-04-06T19:34:45Z

/lgtm

We might want to take a look at kubernetes/test-infra#15611 (comment) to see if we can have tide take a look at the other Github checks.

KnVerey · 2022-04-06T19:51:12Z

Looks like I forgot to apply the "merge-method-squash" and left 23 commits. Sorry! 😬

KnVerey added 5 commits March 31, 2022 19:02

Makefile cleanup

2a9adbe

Makefiles for all modules

0d32543

Run license check in CI

877d72b

Do not change what the presubmit does yet

9fc012c

Delete duplicate copies of the license template

ab9b892

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 1, 2022

k8s-ci-robot requested review from mengqiy and mortent April 1, 2022 21:56

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2022

Lint needs goimports

7440f97

KnVerey force-pushed the rework_ci branch from 56758be to 0536617 Compare April 4, 2022 17:26

Fix root var and add sanity check

08924dc

KnVerey force-pushed the rework_ci branch from 0536617 to 08924dc Compare April 4, 2022 18:03

KnVerey added 3 commits April 4, 2022 14:10

Add latest to go install commands

f54014c

Add helpful error message to license check

3e652d6

Add missing boilerplate

0fa010c

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 4, 2022

KnVerey added 4 commits April 4, 2022 16:46

More makefile cleanup

46875b6

add kustomize module to tests that run on master

af06ae6

Do not run ineffective targets, do run all module tests

2d4bce5

Run all non plugin module builds

ea19332

KnVerey force-pushed the rework_ci branch from c397fcf to 6731677 Compare April 4, 2022 23:39

KnVerey force-pushed the rework_ci branch from 6731677 to 204f3e2 Compare April 5, 2022 00:06

KnVerey force-pushed the rework_ci branch from 204f3e2 to df80b29 Compare April 5, 2022 00:14

KnVerey added 2 commits April 4, 2022 20:40

rever meaning of verify kustomize target

db34e92

less redundancy in presubmit

f68a0c5

KnVerey marked this pull request as ready for review April 5, 2022 01:32

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2022

k8s-ci-robot requested a review from natasha41575 April 5, 2022 01:32

KnVerey changed the title ~~Rework ci~~ Make CI much more effective Apr 5, 2022

KnVerey commented Apr 5, 2022

View reviewed changes

KnVerey mentioned this pull request Apr 5, 2022

fix containerized function mounts issue #4489

Merged

natasha41575 reviewed Apr 5, 2022

View reviewed changes

This was referenced Apr 5, 2022

Raise error for external generators or transformers #4565

Merged

Add support for remote OpenAPI schema #4567

Merged

openapi parsing performance improvement with protobuffer #4568

Merged

KnVerey added 4 commits April 5, 2022 14:31

More makefile cleanup

c071cda

Base makefile and root makefile target for functions/examples

03c94ea

Make sure build targets all install into Go bin

f3c825f

more makefile cleanup

df57e19

KnVerey commented Apr 5, 2022

View reviewed changes

KnVerey added 2 commits April 5, 2022 15:03

Ignore XML in license header script

29104d6

Use verbose mode in all test runs

e96c38e

k8s-ci-robot assigned natasha41575 Apr 6, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2022

k8s-ci-robot merged commit 4fc0249 into kubernetes-sigs:master Apr 6, 2022

KnVerey deleted the rework_ci branch April 6, 2022 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CI much more effective #4564

Make CI much more effective #4564

KnVerey commented Apr 1, 2022 •

edited

k8s-ci-robot commented Apr 1, 2022

k8s-ci-robot commented Apr 1, 2022

k8s-ci-robot commented Apr 1, 2022

KnVerey commented Apr 4, 2022

KnVerey commented Apr 5, 2022

KnVerey commented Apr 5, 2022

KnVerey Apr 5, 2022

KnVerey Apr 5, 2022

KnVerey commented Apr 5, 2022

natasha41575 left a comment

natasha41575 Apr 5, 2022

natasha41575 Apr 5, 2022

KnVerey Apr 5, 2022

natasha41575 Apr 5, 2022

KnVerey Apr 5, 2022

natasha41575 Apr 5, 2022

KnVerey Apr 5, 2022

natasha41575 commented Apr 6, 2022

KnVerey commented Apr 6, 2022

Make CI much more effective #4564

Make CI much more effective #4564

Conversation

KnVerey commented Apr 1, 2022 • edited

k8s-ci-robot commented Apr 1, 2022

k8s-ci-robot commented Apr 1, 2022

k8s-ci-robot commented Apr 1, 2022

KnVerey commented Apr 4, 2022

KnVerey commented Apr 5, 2022

KnVerey commented Apr 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey commented Apr 5, 2022

natasha41575 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 commented Apr 6, 2022

KnVerey commented Apr 6, 2022

KnVerey commented Apr 1, 2022 •

edited