-
Notifications
You must be signed in to change notification settings - Fork 1.6k
🐛 (go/v4): Simplify init directory validation to only block kubebuilder files #5154
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
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 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 |
982e9d3 to
6dcd44b
Compare
6dcd44b to
6d48443
Compare
3449c98 to
f7fede3
Compare
e624b6d to
decd337
Compare
|
It looks a lot more permissive that what is in place at the moment. Will kubebuilder override existing files ? I don't thinks it'll an issue for experienced people since we just have to look a the git diff and reset the files we want to keep as is. But it may be a good idea to add this hack to the documentation, what do you think ? |
|
/hold We need think more about. |
|
I have no idea what amount of work it could represent hut why not adding a step before generating the files ? Something like:
This way, we get rid of the need for hack documentation while keeping flexibility and automated maintenance of the kubebuilder "reserved" files. |
decd337 to
678e1d0
Compare
|
/hold cancel Thank you so much for your patience! 🙏 The previous logic was both inverted and too strict—it didn’t reliably prevent actual conflicts and made maintenance difficult.
Would love your thoughts on this updated approach! ✨ |
| break | ||
| } | ||
| // Skip dot directories | ||
| if info.IsDir() && strings.HasPrefix(info.Name(), ".") { |
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.
Why remove warnings on dot directories?
As a user, I think I would like to be warn if my GitHub workflow was about to be overridden. Or am I missing something?
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! 🙏 I tried to improve it, but just a heads-up — the warning might show up quite a bit, since .idea files (and others) can cause false positives. We have a ton of those.
Please let me know if you spot any important cases that we should actually handle.
Examples:
.github/workflows/ci.yml→⚠️ warning shown.git,.vscode,.idea→ ✅ skipped (no warning)- Only
.gitignore,go.mod,go.sum→ ✅ ignored (no warning)
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's what I was missing ! I forgot that the code doesn't check only for kubebuilder files !
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.
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.
Well. I thinks we should keep the warnings for all the files including the dot files. I feel that skipping them may confuse users.
Imagine the following
- The user tries to initialize in a repo with a .GitHub dir
- They receive a set of warnings, they assumes that the list is complete.
- They accept the warning and get a replaced .github.
If the user doesn't check their git diff, they'll miss the change and could cause issues later on.
This makes sense because nowadays, dot files have very important roles.
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.
.github/workflows/ci.yml seems like the only place where this actually makes
sense. For the other directories, raising a warning doesn’t seem useful and
would just create noise. If we warn on everything, we end up training people
to ignore warnings entirely, which isn’t great practice. It’s better to only
warn when it’s meaningful.
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 right. The best option would be to add a preparation step that list the files about to be created by kubebuilder and check against this list. Maybe such a list could be statically created at build time?
For the time being, I don't know if it is worth the effort. So the solution in this PR could be a quick win. Let's merge!
pkg/plugins/golang/v4/init.go
Outdated
| return nil | ||
| } | ||
| // Track if any non-hidden files/directories exist | ||
| if !strings.HasPrefix(info.Name(), ".") { |
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.
Same here
6e87652 to
eac71b8
Compare
…d files Allow existing config files; only block PROJECT, Makefile, cmd/main.go, and config/default/kustomization.yaml to prevent conflicts.
eac71b8 to
eaf95e3
Compare
|
New changes are detected. LGTM label has been removed. |
Fixes #5143
Problem
kubebuilder initwould fail if the target directory contained commondevelopment tool configuration files like:
go mod initfirst)This forced users to work around the validation by moving files out,
running init, then moving them back.
Solution
Only block initialization if key kubebuilder files already exist:
All other files are allowed. Users get a warning if the directory isn't
empty, but scaffolding proceeds.
Result
Users can now initialize kubebuilder projects in directories with existing
tooling configs without errors.