-
Notifications
You must be signed in to change notification settings - Fork 885
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
implement preserveResourcesOnDeletion to support migration rollback #5597
implement preserveResourcesOnDeletion to support migration rollback #5597
Conversation
9633161
to
90270f8
Compare
21604d0
to
d101b90
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5597 +/- ##
==========================================
+ Coverage 38.25% 38.40% +0.14%
==========================================
Files 649 650 +1
Lines 45133 45207 +74
==========================================
+ Hits 17267 17360 +93
+ Misses 26556 26518 -38
- Partials 1310 1329 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks a lot |
@@ -39,7 +39,7 @@ import ( | |||
) | |||
|
|||
// CreateOrUpdateWork creates a Work object if not exist, or updates if it already exists. | |||
func CreateOrUpdateWork(ctx context.Context, client client.Client, workMeta metav1.ObjectMeta, resource *unstructured.Unstructured, suspendDispatching *bool) error { | |||
func CreateOrUpdateWork(ctx context.Context, client client.Client, workMeta metav1.ObjectMeta, resource *unstructured.Unstructured, options ...WorkOption) 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.
Elegant implementation, learned!
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.
Nice!
Hi @a7i, in addition to clearing labels and annotations, the current pr should be a complete pr. Can we clear labels in a separate pr? (This means that we can advance the integration of the current pr first, and we need to resolve the conflict.) How do you think? |
Signed-off-by: Amir Alavi <amiralavi7@gmail.com>
d101b90
to
5a1ee31
Compare
Hi @XiShanYongYe-Chang 👋🏼 I pushed up a separate commit, let me know if you want it reverted or squashed. I still don't like that labels/annotations are not going to be easily maintainable but I can clean it up in a follow-up PR. Let me know what you think |
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 think we can modify this first, and then optimize label and annotation management in the next pr.
cc @RainbowMango
Signed-off-by: Amir Alavi <amiralavi7@gmail.com>
5a1ee31
to
68c0104
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 your quick response!
/lgtm
/assign @RainbowMango
I will look at it this week. 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.
/approve
Looks great!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grosser, RainbowMango 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Covers function implementation of #5577
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I am open to implementing e2e tests but holding off per @XiShanYongYe-Chang 's recommendation
Does this PR introduce a user-facing change?: