-
Notifications
You must be signed in to change notification settings - Fork 45
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
✨ Add noopdelete option to extension api #772
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
f5efff9
to
2b14aca
Compare
@@ -432,6 +432,10 @@ func (r *ExtensionReconciler) GenerateExpectedApp(o ocv1alpha1.Extension, bundle | |||
}, | |||
} | |||
|
|||
if o.Spec.NoopDelete { |
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.
No need for a conditional here, right?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #772 +/- ##
==========================================
- Coverage 67.90% 67.12% -0.79%
==========================================
Files 22 22
Lines 1424 1466 +42
==========================================
+ Hits 967 984 +17
- Misses 391 416 +25
Partials 66 66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2b14aca
to
7686718
Compare
I ✨ the PR title to pass the pr-title workflow, based on PR contents. This repo enforces naming along these lines: If that's not correct, please choose the option that best fits. |
|
I'm curious if there's a reason we're adding a new feature to the extension API, given that we're planning to focus on the cluster extension API and temporarily remove this API. Should we just make this change directly on the ClusterExtension instead? |
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.
Looks good! /lgtm from my end, after addressing Joe's review.
Regarding surfacing this on the status - looking deeper, it doesn't seem necessary imo. We don't specify anything explicitly for paused state either. Also this info may be useful only when the user tries to delete the relevant extension, at which point we are not going to persist the object itself. Just cross-checked, looks like PackageInstall doesn't have it too.
7686718
to
2276f78
Compare
2276f78
to
d057713
Compare
PR needs rebase. 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. |
Description
Solves #723
Reviewer Checklist