-
Notifications
You must be signed in to change notification settings - Fork 1.6k
✨ (CLI/Api): Add IfNotExistsAction to machinery for optional file handling #4967
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?
✨ (CLI/Api): Add IfNotExistsAction to machinery for optional file handling #4967
Conversation
Hi @aman4433. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
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 great 🥇
Thank you for looking on that.
We need to:
-
Fix lint issues:
Could you please runmake lint-fix
to fix the sort/order of the imports? -
Ensure that we still handle the scenario.
We have no automated tests for this one: #4967 (comment) -
We will need to fix and adapt the unit tests: https://github.com/kubernetes-sigs/kubebuilder/actions/runs/16542895231/job/46797272326?pr=4967
Hey @camilamacedo86, thanks for the suggestion! I’ll work on the changes and push an update soon. |
b54cb0c
to
acce86a
Compare
Just pushed an update — trimmed the scope to handle only missing optional webhook files and cleaned up lint issues. Verified that the previously failing tests are now passing locally. |
/pk-to-test |
Its a typo 😅 |
/ok-to-test |
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 for your PR 🥇
Can you address the recommendations made by Copilot here?
So that we can get this one merged?
45786ba
to
7fa7fdf
Compare
aec3323
to
849a78f
Compare
/test pull-kubebuilder-test |
/approved |
/lgtm cancel |
Hi @aman4433 👋 Thanks so much for the work on this — we're almost there! Just a couple of quick things before we can merge:
Really appreciate your effort on this — it's looking great overall! Let me know if you need a hand with anything 😊 |
272c9b2
to
dfb18f0
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aman4433 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 |
dfb18f0
to
5dd5c95
Compare
Hey @camilamacedo86, Thanks for your support, I think now all the issues are resolved, let me know if this looks good |
Hi @aman4433 We have some fixes |
5dd5c95
to
04f4e7a
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.
Pull Request Overview
This PR introduces a configurable IfNotExistsAction
mechanism to handle optional files during scaffolding operations. Previously, scaffolding would fail when attempting to inject code into missing optional files (like test files), requiring workarounds. The new system allows templates to specify whether missing files should be ignored or cause errors.
- Adds
IfNotExistsAction
enum withErrorIfNotExist
(default) andIgnoreFile
options - Implements
HasIfNotExistsAction
interface andIfNotExistsActionMixin
for templates to use - Updates scaffold logic to check for the new action when files don't exist, replacing hardcoded test file handling
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
pkg/machinery/file.go | Defines the IfNotExistsAction enum and adds the field to the File struct |
pkg/machinery/interfaces.go | Adds HasIfNotExistsAction interface for templates to implement |
pkg/machinery/mixins.go | Provides IfNotExistsActionMixin for easy integration into templates |
pkg/machinery/scaffold.go | Updates file processing logic to use the new action system instead of hardcoded test file handling |
pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook_test_template.go | Example implementation using the new mixin and setting IgnoreFile behavior |
return err | ||
} | ||
} | ||
// If inserter doesn't implement HasIfNotExistsAction, return the original error |
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 comment indicates that if the inserter doesn't implement HasIfNotExistsAction, the original error should be returned, but the code will actually fall through to line 256 which wraps the error in a different format. This inconsistency between comment and implementation could lead to confusion about error handling behavior.
Copilot uses AI. Check for mistakes.
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.
is this comment okay?
// If HasIfNotExistsAction is not implemented, fall through and return a wrapped error.
pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook_test_template.go
Outdated
Show resolved
Hide resolved
04f4e7a
to
11dcd98
Compare
This PR introduces a new mechanism to gracefully handle optional files during scaffolding using a configurable
IfNotExistsAction
.Currently, scaffolding fails when an optional file (like
test/e2e/e2e_test.go
) is missing. This is problematic for plugins/templates that want to inject content only if the file exists.Fixes: #4960