-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
🐛 (go/v3): remove a no longer applied comment from Makefile #2808
🐛 (go/v3): remove a no longer applied comment from Makefile #2808
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have one concern with this change. I do agree that the comment stating:
This is a requirement for 'setup-envtest.sh' in the test target
should be removed but I am not sure entirely removing the block that sets the SHELL
is a good idea due to some potential issues.
# Setting SHELL to bash allows bash commands to be executed by recipes. | ||
# This is a requirement for 'setup-envtest.sh' in the test target. | ||
# Options are set to exit when a recipe line exits non-zero or a piped command fails. | ||
SHELL = /usr/bin/env bash -o pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern with this change is that while the comment states that This is a requirement for 'setup-envtest.sh' in the test target.
, are there any other scripts or anything of the sort used throughout the Makefile that uses bash
specific syntax? A quick check shows that the script used to install Kustomize is a bash
script and likely uses bash
specific syntax:
kubebuilder/testdata/project-v4/Makefile
Line 119 in 33ad938
KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" |
It's also my understanding that it is best practice to specify the SHELL in a Makefile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I am not a shell expert, nor on makefiles. Ref. my comment in the PR description:
I think at least the comment should be deleted, but I suggest to remove the whole block.
Should I just remove the misleading comment then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikgb I am all for removing just the comment. If that is the only change then I think it is pretty trivial and I would be happy to approve the PR after that! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the manual migration docs? Leave it as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR to just remove the misleading comment. PTAL!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the manual migration docs is okay to leave as is.
Will re-review the new changes now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR only removes
> # This is a requirement for 'setup-envtest.sh' in the test target.
We no longer use the 'setup-envtest.sh' so I think that is 100 fine to move forward. 👍
I think we cannot remove the SHELL code for the reasons pointed out by @everettraven.
We are using the shell to install the kustomize.
e54e190
to
1e2b4b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @erikgb ! This looks good to me!
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @erikgb,
Good catcher one this one 👍
The only thing is that change the scaffolds so would be better to change the title like
🐛 (go/v3): remove a comment that is no longer applied from Makefile.
I hope that you do not mind I will update the title for we get this one merged.
/approved
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, erikgb, everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I just created a new project with kubebuilder, and got confused over a comment in the scaffolded Makefile:
This was added in #2149, but later replaced with
go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
.