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

Make install target is broken because try to install the kustomize when it is installed already #5875

Closed
camilamacedo86 opened this issue Jun 14, 2022 · 7 comments
Assignees
Milestone

Comments

@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Jun 14, 2022

Bug Report

What did you do?

call make install twice

What did you expect to see?

Not fail and just install the kustomize if that is not installed already

What did you see instead? Under which circumstances?

The following error in the second time because the bin is in the dir already

$ make install
/Users/camilamacedo86/go/src/guestbook/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
curl -s "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash -s -- 3.8.7 /Users/camilamacedo86/go/src/guestbook/bin
/Users/camilamacedo86/go/src/guestbook/bin/kustomize exists. Remove it first.
make: *** [Makefile:123: /Users/camilamacedo86/go/src/guestbook/bin/kustomize] Error 1

Environment

Operator type:

/language go
/language ansible
/language helm

$ operator-sdk version

master

@openshift-ci
Copy link

openshift-ci bot commented Jun 14, 2022

@camilamacedo86: The label(s) language/--> cannot be applied, because the repository doesn't have them.

In response to this:

Bug Report

What did you do?

call make install twice

What did you expect to see?

Not fail and just install the kustomize if that is not installed already

What did you see instead? Under which circumstances?

The following error in the second time because the bin is in the dir already

$ make install
/Users/camilamacedo86/go/src/guestbook/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
curl -s "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash -s -- 3.8.7 /Users/camilamacedo86/go/src/guestbook/bin
/Users/camilamacedo86/go/src/guestbook/bin/kustomize exists. Remove it first.
make: *** [Makefile:123: /Users/camilamacedo86/go/src/guestbook/bin/kustomize] Error 1

Environment

Operator type:

/language go -->
/language ansible -->
/language helm

$ operator-sdk version

master

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.

@openshift-ci openshift-ci bot added the language/ansible Issue is related to an Ansible operator project label Jun 14, 2022
@camilamacedo86
Copy link
Contributor Author

c/c @ryantking

@camilamacedo86 camilamacedo86 removed the language/ansible Issue is related to an Ansible operator project label Jun 14, 2022
@jchunkins
Copy link

jchunkins commented Jun 21, 2022

This seems like the make target for installing kustomize just needs to be changed to this simple bash expression:

KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh"
.PHONY: kustomize
kustomize: $(KUSTOMIZE) ## Download kustomize locally if necessary.
$(KUSTOMIZE): $(LOCALBIN)
	test -s $(LOCALBIN)/kustomize || { curl -s $(KUSTOMIZE_INSTALL_SCRIPT) | bash -s -- $(subst v,,$(KUSTOMIZE_VERSION)) $(LOCALBIN); }

In case it helps see before/after:

--- a/Makefile
+++ b/Makefile
@@ -168,7 +168,7 @@ KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/k
 .PHONY: kustomize
 kustomize: $(KUSTOMIZE) ## Download kustomize locally if necessary.
 $(KUSTOMIZE): $(LOCALBIN)
-       curl -s $(KUSTOMIZE_INSTALL_SCRIPT) | bash -s -- $(subst v,,$(KUSTOMIZE_VERSION)) $(LOCALBIN)
+       test -s $(LOCALBIN)/kustomize || { curl -s $(KUSTOMIZE_INSTALL_SCRIPT) | bash -s -- $(subst v,,$(KUSTOMIZE_VERSION)) $(LOCALBIN); }

@jberkhahn
Copy link
Contributor

TODO: do we need to bump kubebuilder to get this?

@asmacdo asmacdo modified the milestones: v1.23.0, v1.24.0 Jul 20, 2022
@asmacdo
Copy link
Member

asmacdo commented Jul 20, 2022

Still need to bump kubebuilder it seems, we updated to 3.5.0, which i assume doesnt have this change @camilamacedo86 ?

@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Jul 21, 2022

@asmacdo we can check that the change was made in the PR linked: kubernetes-sigs/kubebuilder#2774

Therefore, we can see that it is only in master and that we do have not an update here on the SDK side, see the SDK testdata samples:

KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh"
.PHONY: kustomize
kustomize: $(KUSTOMIZE) ## Download kustomize locally if necessary.
$(KUSTOMIZE): $(LOCALBIN)
est -s $(LOCALBIN)/kustomize || { curl -s $(KUSTOMIZE_INSTALL_SCRIPT) | bash -s -- $(subst v,,$(KUSTOMIZE_VERSION)) $(LOCALBIN); }

We can do that now by updating the master commit OR we can wait for the next KB release. Indeed we do many times bump using the commits. Would you like to do this one? If yes, please feel free to do the bump and I can help with the reviews.

@camilamacedo86
Copy link
Contributor Author

Sorted out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants