Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Download and use controller-gen and envtest locally #369

Conversation

cebernardi
Copy link
Contributor

If we download the project in a local environment with no kubebuilder and kubebuilder-tools, or with an outdated version of controller-gen, executing make test will fail (see also this comment).

This PR copies the approach of latest version of kubebuilder, downloading the executables in a local directory and consuming them from there.

(since apparently there's no release for envtest, I had to use latest)

Signed-off-by: Cecilia Bernardi cbernardi@expediagroup.com

@stefanprodan
Copy link
Member

I used a different approach in kustomize-controller where envtest is not used from the shared dir. I would like to get this in all Flux controllers, please see https://github.com/fluxcd/kustomize-controller/blob/main/Makefile

@cebernardi
Copy link
Contributor Author

@stefanprodan thank you for the feedback!
I'm adapting this PR to your suggestions.

@cebernardi
Copy link
Contributor Author

cebernardi commented Dec 9, 2021

@stefanprodan I modified the PR following your comment.

However, there are some differences from the Makefile you linked, the main ones being that both controller-gen and envtest are downloaded in a local folder (bin). This was copied by the latest kubebuilder approach.

What are your thoughts on the proposed changes?

@stefanprodan
Copy link
Member

@cebernardi this looks great, please squash all commits into a single one.

@stefanprodan stefanprodan added the area/ci CI related issues and pull requests label Dec 9, 2021
@cebernardi cebernardi force-pushed the download-locally-controller-gen-and-testenv branch from 925dc84 to bd4017d Compare December 9, 2021 15:57
@cebernardi
Copy link
Contributor Author

@stefanprodan please ignore my last commit, it's a rebase fail :(
still working on it

@cebernardi cebernardi force-pushed the download-locally-controller-gen-and-testenv branch 2 times, most recently from 6379ca4 to 79dcd22 Compare December 9, 2021 16:52
@cebernardi
Copy link
Contributor Author

@stefanprodan all the commits are now squashed

@stefanprodan
Copy link
Member

This fails for me on Apple Silicon and Go 1.17:

go: creating new go.mod: module tmp
Downloading sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
go: downloading sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20211208212546-f236f0345ad2
go get: installing executables with 'go get' in module mode is deprecated.
	To adjust and download dependencies of the current module, use 'go get -d'.
	To install using requirements of the current module, use 'go install'.
	To install ignoring the current module, use 'go install' with a version,
	like 'go install example.com/cmd@latest'.
	For more information, see https://golang.org/doc/go-get-install-deprecation
	or run 'go help get' or 'go help install'.
go get: added sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20211208212546-f236f0345ad2
mkdir -p /Users/stefanprodan/go/src/github.com/fluxcd/helm-controller/testbin
/Users/stefanprodan/go/src/github.com/fluxcd/helm-controller/bin/setup-envtest use latest --bin-dir=/Users/stefanprodan/go/src/github.com/fluxcd/helm-controller/testbin
unable to find a version that was supported for platform darwin/arm64
make: *** [Makefile:127: install-envtest] Error 2

@hiddeco can you please try this on your Intel machine?

@stefanprodan
Copy link
Member

Ok I got this running:

KUBEBUILDER_ASSETS="/Users/stefanprodan/go/src/github.com/fluxcd/helm-controller/testbin/k8s/1.22.1-darwin-amd64" go test ./... -coverprofile cover.out
?   	github.com/fluxcd/helm-controller	[no test files]
ok  	github.com/fluxcd/helm-controller/controllers	12.422s	coverage: 20.5% of statements
?   	github.com/fluxcd/helm-controller/internal/kube	[no test files]
ok  	github.com/fluxcd/helm-controller/internal/runner	0.871s	coverage: 28.1% of statements
ok  	github.com/fluxcd/helm-controller/internal/util	0.190s	coverage: 100.0% of statements

I'll suggest changes.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please pin envtest to AMD64

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@cebernardi
Copy link
Contributor Author

should I squash again?

@stefanprodan
Copy link
Member

Yes please squash

@hiddeco
Copy link
Member

hiddeco commented Dec 9, 2021

I would maybe still make it a configurable flag to ease the first time usage if ARM64 ever becomes available.. E.g. ENVTEST_ARCH?=amd64

@cebernardi cebernardi force-pushed the download-locally-controller-gen-and-testenv branch 2 times, most recently from 7b590b6 to fb8f738 Compare December 9, 2021 17:34
* controller-gen and envtest are downloaded to a `bin` folder, local to the project, and used from there
* envtest assets are installed in a `testbin` folder, local to the project, and used from there

Signed-off-by: Cecilia Bernardi <cbernardi@expediagroup.com>
@cebernardi cebernardi force-pushed the download-locally-controller-gen-and-testenv branch from fb8f738 to 1d03726 Compare December 9, 2021 17:36
@cebernardi
Copy link
Contributor Author

I would maybe still make it a configurable flag to ease the first time usage if ARM64 ever becomes available.. E.g. ENVTEST_ARCH?=amd64

done

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me, and a very much welcomed improvement. Thanks a lot @cebernardi
🙇 🥇

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @cebernardi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci CI related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants