-
Couldn't load subscription status.
- Fork 1.6k
✨ (go/v4): Add allowed files about tools version management files in go init v4 #5143
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
base: master
Are you sure you want to change the base?
✨ (go/v4): Add allowed files about tools version management files in go init v4 #5143
Conversation
…it v4 Signed-off-by: Robin LIORET <[email protected]>
|
Welcome @robinlioret! |
|
Hi @robinlioret. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
pkg/plugins/golang/v4/init.go
Outdated
| allowedFiles := []string{ | ||
| // User might use tool versions management tools to set up the environment including kubebuilder and go version | ||
| "mise.toml", // mise-en-place configuration file | ||
| ".tool-versions", // asdf configuration file |
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.
Thank you so much for raising this one 🎉
Could we check why it’s being blocked instead?
It seems we shouldn’t block these since they’re not in disallowedExtensions.
If we start creating a full list like that, it could get huge —
so it might be better to only block the specific required cases.
What do you think? Could you please take a look? 🙂
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.
You're welcome!
Actually, I think the variable disallowedExtensions is misnamed. It should be allowedExtensions since it is treated as a guard clause before returning the error.
I agree, init should block the files that are created by kubebuilder and leave the rest be. But, maybe there were a reason why this was implemented in the first place.
I'll dig into the repo to find out why andupdate this PR.
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.
Thank you a lot !!!. Sure, the reason is:
We cannot initialize or scaffold files when the target directory already contains files that the tool itself is meant to scaffold. Doing so can lead to conflicts during the command execution — and, even worse, it may overwrite existing files with different content.
To avoid these issues, the initialization process must ensure that the directory is either empty or does not contain files that overlap with those that will be generated by the scaffolding tool.
Signed-off-by: Robin LIORET <[email protected]>
Signed-off-by: Robin LIORET <[email protected]>
Signed-off-by: Robin LIORET <[email protected]>
|
I reworked the function so it only checks for kubebuilder reserved files and directories. make install
mkdir testdata/project-v4-invalid-dir && cd testdata/project-v4-invalid-dir
touch .devcontainer Makefile mise.toml .tools-version go.mod
mkdir accepted && touch accepted/Makefile
kubebuilder init --domain=dummy.io --repo=github.com/dummy-io --license=apache2 --plugins go/v4
#> target directory contains kubebuilder reserved directory or file: ".devcontainer" (.devcontainer, .github, api, bin, cmd, config, dist, hack, internal, test, .dockerignore, .gitignore, .golangci.yml, Dockerfile, Makefile, PROJECT, README.md, grafana)
rm .devcontainer
kubebuilder init --domain=dummy.io --repo=github.com/dummy-io --license=apache2 --plugins go/v4
#> target directory contains kubebuilder reserved directory or file: "Makefile" (...)
rm Makefile
kubebuilder init --domain=dummy.io --repo=github.com/dummy-io --license=apache2 --plugins go/v4
# No error
make generate
# It works
cd ..
rm -rf project-v4-invalid-dir |
Signed-off-by: Robin LIORET <[email protected]>
| ".sum", | ||
|
|
||
| // Do not allow kubebuilder reserved directories or files | ||
| reservedNames := []string{ |
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.
That is definitely an option.
I will need some time to think about it.
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 main issue here is how to maintain this list.
Moreover: In a multi-repository project, there will typically be a .github directory — and that’s probably fine. However, the tool would need to scaffold files inside that directory, which raises the question of how to manage and keep the list consistent across repositories.
| ".sum", | ||
|
|
||
| // Do not allow kubebuilder reserved directories or files | ||
| reservedNames := []string{ |
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 main issue here is how to maintain this list.
Moreover: In a multi-repository project, there will typically be a .github directory — and that’s probably fine. However, the tool would need to scaffold files inside that directory, which raises the question of how to manage and keep the list consistent across repositories.
I need time to evaluate not sure if that is the best approach
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: robinlioret The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I understand, I'm not experienced enough with more complex usage of Kubebuilder. This approch is the opposite of the previous one. May be it is possible to dynamically build the list from the templates. |
Motivation
I'm using mise-en-place to manage my tools versions. When I want to create a new kubebuilder project. I start by creating a
mise.tomlfile with the versions of go and kubebuilder I want to use. Then run thekubebuilder init ...command. Since .toml files are allowed, I get the errorI would like to be able to use mise tool stack, so we need to allow tools management files like mise-en-place and asdf.
Description of the change
Adding a simple check of the filename against an hardcoded list. It's the same principle as for the extension check but against the whole filename.
Prerequisites