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

Missing Kustomize Dependency for make install #1371

Closed
krisnova opened this issue Feb 14, 2020 · 20 comments
Closed

Missing Kustomize Dependency for make install #1371

krisnova opened this issue Feb 14, 2020 · 20 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug.

Comments

@krisnova
Copy link

Fresh linux system, trying to go through the getting started

[nova@nova kapad]$ make install
/home/nova/go/bin/controller-gen "crd:trivialVersions=true" rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
kustomize build config/crd | kubectl apply -f -
/bin/sh: kustomize: command not found
error: no objects passed to apply
make: *** [Makefile:30: install] Error 1

Either docs that mention this, a check in make install, a better error message with a path forward, or including kustomize in the build/bin/ directory of a release

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 14, 2020
@droot
Copy link
Contributor

droot commented Feb 27, 2020

Kubebuilder book mentions kustomize in prerequisites section in quick-start-guide.

Though better error message pointing to kustomize install page would be great help here.

@droot droot added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Feb 27, 2020
@bharathi-tenneti
Copy link
Contributor

/assign

@bharathi-tenneti
Copy link
Contributor

bharathi-tenneti commented Mar 6, 2020

@droot
Did you mean the check to be done in this file?
P.S New to this repo.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Mar 9, 2020

HI @bharathi-tenneti,

It is not about the makefile of the kubebuilder project. The makefile in the kubebuilder project are targets which are helpful for who will contribute with the project.

In this scenario, the user says Fresh linux system, trying to go through the getting started which means that he is following the Quick Started and the issue is faced in the step:

Screenshot 2020-03-09 at 19 07 23

So, what @droot is suggesting here as the solution is:

Improve the error face in this scenario in order to allow users to know that the prerequisite kustomize is not installed. Also, I would recommend we solve it by checking the pre-requirement is install as we do for the Go version see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/cmd/internal/go_version.go

@Adirio
Copy link
Contributor

Adirio commented Mar 10, 2020

I agree with @camilamacedo86 that checking the dependency could be done. I don't think there is any other easy way to improve the error message, as it is a OS error not one we are printing, so we need to check for it before running any kustomize command.

@bharathi-tenneti
Copy link
Contributor

@Adirio I agree that checking dependency can be done for kustomize.
However, if we go down the path, Should we go ahead an check for other pre-reqs as well?
Do we want to do that ?

@camilamacedo86
Copy link
Member

@bharathi-tenneti,

Feel free to do. the. other checks if you see that would be nice. To close this one IHMO doing it for kustomize only would be enough.

@Adirio
Copy link
Contributor

Adirio commented Mar 11, 2020

I also wanted to move this check out of kubebuilder init just to any kubebuilder command so that when you git clone an already init project you still get this check done for you.
I was working on this, but don't have the PR ready yet. Feel free to submit a PR with the fix, I didn't realize you had it assigned to yourself. I will do then the PR moving the checks to any command and also a small refactor to the go version check.

dmizelle added a commit to dmizelle/kubebuilder that referenced this issue Mar 12, 2020
This commit checks that the prequisites listed in the Kubebuilder book
are in `$PATH` before proceeding with any target.

We probably want the user to get better feedback up front instead of
`make` failing half-way through (possibly)

Fixes kubernetes-sigs#1371
dmizelle added a commit to dmizelle/kubebuilder that referenced this issue Mar 12, 2020
This commit checks that the prerequisites listed in the Kubebuilder book
are in `$PATH` before proceeding with any target.

We probably want the user to get better feedback up front instead of
`make` failing half-way through (possibly)

Fixes kubernetes-sigs#1371
@dmizelle
Copy link

Eek. I didn't realize this was assigned to someone else (its 2AM here and I'm unable to sleep)

Didn't mean to step on someone else that was interested in doing this. I ran into this issue tonight as I just rely on kustomize built into kubectl usually. If you've already done this (or are working on it) just feel free to close my PR. :)

@bharathi-tenneti
Copy link
Contributor

@dmizelle no worries, I have not got around to it yet, so go ahead.

@Adirio
Copy link
Contributor

Adirio commented Mar 12, 2020

He made a proposal on #1427 and I made mine in #1428.

@toonsevrin
Copy link
Contributor

Once #1430 is merged, kustomize will be installed automatically when not present in PATH.

@Adirio
Copy link
Contributor

Adirio commented Mar 14, 2020

So we could either install or just check for the dependency. Both aproaches are being used currently: go version is checked while controller-gen is installed if not present. I'm not sure which of them feels better.

@mengqiy @droot @DirectXMan12 what approach do you prefer?

  • Install the dependency: @camilamacedo86
  • Check for the dependency but delegate installing to user:

@camilamacedo86
Copy link
Member

Hi @Adirio,

I think that go is a pre-requirement that could not be changed. However, doing it by the Makefile allows the user to customize it which shows more appropriate in this case. So, I am inclined to say for we forward with #1430.

@toonsevrin
Copy link
Contributor

I've commented this on #1430, but ideally the dependencies like controller-gen and kustomize should only be installed locally (eg. local to the project). This could be in ${PROJECT_DIR}/bin for example.

As they are internal dependencies in my opinion, they should not change the external environment (and ideally not be influenced by it either, all though that would break backwards compatibility).

Anyways, this is perfectionistic, so I would simply go with #1430 as well!

@bharathi-tenneti bharathi-tenneti removed their assignment Mar 15, 2020
@Adirio
Copy link
Contributor

Adirio commented Mar 15, 2020

I do agree that having a bin folder with local dependencies sounds better than globally installed. But until we opt into thta approach, I would not install things for users, I would just check if they are installed. Basically because we are changing their global system and not just our local directory.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Mar 15, 2020

IHMO, the kustomize is a requirement dependence such as the controller-gen and the solution in #1430 make it easier for the users and bring better experience, also address the issue raised here and still allowing users customize it according to their needs since the implementation is in the makefile. Then, I cannot see any reason why not move forward with #1430.

@mengqiy @DirectXMan12 wdyt ?

@Adirio
Copy link
Contributor

Adirio commented Mar 23, 2020

@droot @mengqiy @DirectXMan12 would you mind answering the poll raised in this #1371 (comment)?

@camilamacedo86
Copy link
Member

/close

It was solved with #1430

@k8s-ci-robot
Copy link
Contributor

@camilamacedo86: Closing this issue.

In response to this:

/close

It was solved with #1430

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
8 participants