✨ Improve developer tooling #382

tomasaschan · 2025-03-12T16:47:17Z

The purpose of this PR is to make a bunch of improvements to the developer tooling, over what was scaffolded by Kubebuilder and hacked together on top of that. For more discussion, see #372.

Guiding principles

These are a couple of principles I'm trying to use for guidance on how to set things up:

Expose tools through `go tool`

By adding tools as dependencies in go.mod, through go get -tool ..., we make it so that as little as possible, other than a modern Go toolchain, is required to start contributing to the project. In addition,
this makes it easier for automation to manage version bumps etc.

Make as much as possible go through the `go` toolchain

We should be able to run tests with go test ./..., generate code and other artifacts using go generate ./..., and so on. The dependency on a Makefile to document how to run third-party tools is a smell.

Sometimes it cannot be avoided; e.g. there is currently no way to hook into go vet that would allow us to configure e.g. golangci-lint and govulncheck to run through that, but where possible we should try to avoid using other CLI entrypoints than go itself.

One True Way

There should only be one way to do something; for example, today we have the config directory which exposes a way to assemble all the config required to deploy kro by means of kustomize build, but we also have a Helm chart that results in similar but not identical config. We should unify those (and find other cases of "more than one way to do stuff") so that if you figured out how you can do something, you also figured out how you should do it.

Progress and ideas

We'll see how much of this I do in this PR and how much will be punted for later. The diff is already quite large, but basically anything that's not in the tool section of go.mod, or in the new files codegen.go and .nwa-config.yaml, is generated.

tomasaschan · 2025-03-13T15:02:56Z

@a-hilaly The tests are now failing because the bump of sigs.k8s.io/controller-runtime from ae0b66d also brings in github.com/google/[email protected] which seems to have a breaking change compared to what the tests assume. Not sure I understand enough of what's going on in those tests to fix them 😞 Do you have any input?

tomasaschan · 2025-03-14T09:23:31Z

@a-hilaly I figured out the test failures; #388 should fix it. I'll incorporate that change here to unblock this work.

tomasaschan · 2025-03-14T15:22:52Z

I think this is ready for a proper review now!

The change set is huge, because the automated changes touch almost every file. Most of it is whitespace changes in the license headers (🤦🏻) but I've tried to separate out the generated changes into separate commits to make them easier to ignore when reviewing.

There were also a few tools being mentioned and installed that weren't actually used anywhere - e.g. cosign and govulncheck; I'm assuming here that those were installed aspirationally, and I'm punting adding those into the project for a future time when we actually want to integrate them in the other workflows.

Most of the local development workflow should now be covered by go <cmd> - e.g. go vet ./..., go test ./..., go generate ./... and go run ./...; most of what is not possible to with native commands, but which you might want to do locally anyway, is encapsulated in go tool, e.g. go tool golangci-lint run or go tool ko build ....

Some things, e.g. releasing new versions, is no longer "easy" to do locally, because I feel they shouldn't be done locally - they should be done from GH actions, in response to merges or tags being pushed. That's why basically all helm stuff is just removed.

(I realize as the build turns red that I might have to iterate a couple of times on the GH actions here... please bear with me!)

tomasaschan · 2025-03-14T22:03:53Z

(There; rebased to get rid of conflicts in go.{mod,sum} after merging #388)

This relies on yet unreleased features of controller-runtime, but it will we'll be able to roll forward to the next patch release as soon as one that contains both kubernetes-sigs/controller-runtime#3135 and kubernetes-sigs/controller-runtime#3137 is released.

n3wscott · 2025-03-19T16:57:42Z

config/crd/kustomization.yaml

are these deleted because they are no longer generated?

Correct; we now put all generated manifests (and some hand-crafted) in the helm chart directly, so there is no longer any kustomization going on.

(Note that there is a corresponding CRD file generated in helm/templates/crds; the rest of the stuff in here is basically unused in the default scaffolding from kubebuilder.)

n3wscott · 2025-03-19T17:02:25Z

go.mod

+	k8s.io/apiextensions-apiserver v0.33.0-beta.0
+	k8s.io/apimachinery v0.33.0-beta.0
+	k8s.io/apiserver v0.33.0-beta.0
+	k8s.io/client-go v0.33.0-beta.0


might not want to upgrade to a pre release of client go yet.

n3wscott · 2025-03-19T17:02:43Z

go.mod

+	k8s.io/apiserver v0.33.0-beta.0
+	k8s.io/client-go v0.33.0-beta.0
+	k8s.io/kube-openapi v0.0.0-20250304201544-e5f78fe3ede9
+	sigs.k8s.io/controller-runtime v0.20.1-0.20250313041745-d82dcd80ca96


Likely want to roll this back to a stable release

Both client-go and controller-runtime are bumped to pre-release to get access to kubernetes-sigs/controller-runtime#3135 (and the follow-up fix 3137); they should be released in v0.20.4 shortly, and then we should use that.

n3wscott · 2025-03-19T17:04:21Z

test/integration/environment/setup.go

@@ -66,15 +68,16 @@ func New(controllerConfig ControllerConfig) (*Environment, error) {
 	env.TestEnv = &envtest.Environment{
 		CRDDirectoryPaths: []string{
 			// resourcegraphdefinition CRD
-			filepath.Join("../../../..", "config", "crd", "bases"),
+			filepath.Join("..", "..", "..", "..", "helm", "templates", "crds"),


this is a good catch, the old way would make windows sad

a-hilaly · 2025-04-01T01:34:51Z

Hi @tomasaschan - are you still planning on breaking down this PR into a fewer smaller ones? if not we can have someone pick it up and make sure commits are co-authored with your email.

tomasaschan · 2025-04-01T14:56:40Z

@a-hilaly Yes, I've been meaning to follow up on this! But I've held off for a couple of reasons - partly because I've been swamped at work, but also because this question in Slack never really got answered.

In short: this PR uses https://github.com/B1NARY-GR0UP/nwa to manage license headers, but in the Community Meeting a couple of weeks ago we discussed using https://github.com/google/addlicense/ instead; I'm thinking google/addlicense#173 is somewhat of a blocker for that, and want input from the maintainers on how to proceed.

tomasaschan force-pushed the devtools branch 5 times, most recently from 13333f8 to 3f3ae20 Compare March 13, 2025 13:19

tomasaschan force-pushed the devtools branch from 593610c to 39d7579 Compare March 14, 2025 09:26

tomasaschan marked this pull request as ready for review March 14, 2025 15:11

tomasaschan force-pushed the devtools branch 2 times, most recently from 9643011 to 79e3f03 Compare March 14, 2025 22:03

tomasaschan force-pushed the devtools branch from 79e3f03 to c5bd21c Compare March 17, 2025 07:48

tomasaschan added 16 commits March 19, 2025 15:41

Configure attribution-gen

67714c0

[automated] Run go generate ./...

cf5ea84

Configure nwa

d0264c7

[automated] Run go generate ./...

1e443f3

Configure controller-gen for object/deepcopy

3557734

[automated] Run go generate ./...

71aaa24

Set up controller-gen to generate the RBAC config in the Helm chart

25cf311

Remove a bunch of now-obsolete stuff

9815a48

Update Github actions to reflect tooling changes

3dbfe1b

[automated] Run go generate ./...

13d4a5c

Configure golangci-lint as a Go tool

5bf9b4c

[mostly automated] Fix everything golangci-lint complains about

da36fc8

Remove yq from toolchain; it's not referenced anywhere

a8ab97c

[automated] Run go generate ./...

c69f611

Add some devdocs on the simplified tooling setup

ba6bd79

tomasaschan added 10 commits March 19, 2025 15:42

Improve devdocs to not rely on make target

3c4b80b

Remove build-image make targets and make adjustments accordingly

96f848c

Port helm tasks to not need Make

768c532

Remove no-longer-needed hack folder

dd330c5

Fix build?

8eed6cc

Fix test runner checks and remove redundant checks from image action

c62f534

Escape newlines instead of trying to remove them

a0bf20a

Port remaining config aspects over to Helm chart

2808b01

Recover accidentally left-out CRD generation

769d7df

Run go generate ./... after rebase

64e8eda

tomasaschan force-pushed the devtools branch from c5bd21c to 64e8eda Compare March 19, 2025 14:44

n3wscott reviewed Mar 19, 2025

View reviewed changes

tomasaschan mentioned this pull request Mar 20, 2025

devtools(1/many): Generate ATTRIBUTION.md with go generate #437

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Improve developer tooling #382

✨ Improve developer tooling #382

tomasaschan commented Mar 12, 2025 •

edited

Loading

tomasaschan commented Mar 13, 2025

tomasaschan commented Mar 14, 2025

tomasaschan commented Mar 14, 2025

tomasaschan commented Mar 14, 2025

n3wscott Mar 19, 2025

tomasaschan Mar 19, 2025

tomasaschan Mar 19, 2025

n3wscott Mar 19, 2025

n3wscott Mar 19, 2025

tomasaschan Mar 19, 2025

n3wscott Mar 19, 2025

a-hilaly commented Apr 1, 2025

tomasaschan commented Apr 1, 2025

✨ Improve developer tooling #382

Are you sure you want to change the base?

✨ Improve developer tooling #382

Conversation

tomasaschan commented Mar 12, 2025 • edited Loading

Guiding principles

Expose tools through go tool

Make as much as possible go through the go toolchain

One True Way

Progress and ideas

tomasaschan commented Mar 13, 2025

tomasaschan commented Mar 14, 2025

tomasaschan commented Mar 14, 2025

tomasaschan commented Mar 14, 2025

n3wscott Mar 19, 2025

Choose a reason for hiding this comment

tomasaschan Mar 19, 2025

Choose a reason for hiding this comment

tomasaschan Mar 19, 2025

Choose a reason for hiding this comment

n3wscott Mar 19, 2025

Choose a reason for hiding this comment

n3wscott Mar 19, 2025

Choose a reason for hiding this comment

tomasaschan Mar 19, 2025

Choose a reason for hiding this comment

n3wscott Mar 19, 2025

Choose a reason for hiding this comment

a-hilaly commented Apr 1, 2025

tomasaschan commented Apr 1, 2025

tomasaschan commented Mar 12, 2025 •

edited

Loading

Expose tools through `go tool`

Make as much as possible go through the `go` toolchain