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

Remove external kustomize dependency #1426

Closed
toonsevrin opened this issue Mar 11, 2020 · 11 comments · Fixed by #1430
Closed

Remove external kustomize dependency #1426

toonsevrin opened this issue Mar 11, 2020 · 11 comments · Fixed by #1430
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@toonsevrin
Copy link
Contributor

As kustomize is integrated in kubectl nowadays (without an external dependency), I recommend kubebuilder does the same.

I propose that kustomize becomes an internal go dependency and will provide some documentation here for one of the developers to implement this.

Here is a simple example of using kustomize as a go dependency. Here is the specific line where a resource is generated with the kustomize go library.

As external behavior shouldn't change for this, it can be implemented in v2. Good luck!

@toonsevrin toonsevrin added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 11, 2020
@toonsevrin toonsevrin changed the title Remove environment kustomize dependency Remove external kustomize dependency Mar 11, 2020
@Adirio
Copy link
Contributor

Adirio commented Mar 11, 2020

Kustomize dependency is due to the scaffolded Makefile which has some targets that use kustomize ... commands. How would you suggest to do this?

P.S.: modifying the scaffolded Makefile is definetely a backwards incopatible change that will require a major version bump.

@toonsevrin
Copy link
Contributor Author

I see, I didn't know it was used in the Makefile itself (but makes sense).

I just took a look at the makefile and why is kustomize not injected like the controller-gen?

Perhaps this is a good first step? :) Don't understand why this would break any makefile behavior though

@Adirio
Copy link
Contributor

Adirio commented Mar 11, 2020

I couldn't look the links you provided, if you still have access to kustomize command from the Makefile it will be backwards compatible, just wanted to note that if you had to modify it it will be for v3.

@toonsevrin
Copy link
Contributor Author

toonsevrin commented Mar 11, 2020

@Adirio all right, please provide me with:

  • The location of the Makefile template (so I can update it to add this logic)
  • The version of kustomize you would like to use (It should be pinned to a version for consistency and checksum security)

And I'll update the Makefile template to go get kustomize if which kustomize does not resolve, keeping backwards compatibility in mind.

@Adirio
Copy link
Contributor

Adirio commented Mar 11, 2020

The Makefile template is under pkg/scaffold/internal/templates/v2/makefile.go, but do you need to modify it? If you modify it, then this won't be doable for v2.
Kustomize version is specified in the kubebuilder book.

@toonsevrin
Copy link
Contributor Author

toonsevrin commented Mar 11, 2020 via email

@Adirio
Copy link
Contributor

Adirio commented Mar 11, 2020

A project initialized (kubebuilder init) will kubebuilder version 2.3.0 (and thus have the old Makefile) should be able to update kubebuilder to 2.4.0 and still behave the same.

@toonsevrin
Copy link
Contributor Author

Hm, I see, @Adirio. Note that the old happy path won't change at all (due to the which kustomize check).

Anyways, if you feel that this is for v3 nonetheless, then we can simply add this issue to the roadmap.

@Adirio
Copy link
Contributor

Adirio commented Mar 12, 2020

If the path doesn't change we won't need an update. It was just a comment, do not take it into account, push a PR and we will be able to eval if it needs a major version bump.

@toonsevrin
Copy link
Contributor Author

All right, the only path that changes is in case kustomize is not installed in your environment (In this case it will be loaded in).

@toonsevrin
Copy link
Contributor Author

This issue should be closed once #1430 (resolves this) merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants