Skip to content
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

log error, instead of returning when vs deletion fails #254

Merged
merged 3 commits into from
Aug 29, 2022

Conversation

nirvanagit
Copy link
Collaborator

Goal

When a replicated VS is deleted in the source cluster, it gets deleted in clusters where it is replicated in. During this process, if the deletion fails due to some reason, it skips deleting the vs from the remaining clusters.

This PR changes that behavior, and makes it consistent with deletion workflow for DestinationRules, and ServiceEntries, where it logs that there was an error and continues deleting the resource in other clusters.

@nirvanagit nirvanagit requested review from shriramsharma and aattuluri and removed request for shriramsharma August 26, 2022 16:20
Copy link
Contributor

@aattuluri aattuluri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good. Should we add a unit with mock controller one to deleting an existing resource and another to delete a non existing one?

@nirvanagit nirvanagit force-pushed the vs-deletion branch 3 times, most recently from 3d22278 to e1fbcde Compare August 26, 2022 19:17
Anubhav Aeron added 2 commits August 26, 2022 12:26
Signed-off-by: Anubhav Aeron <anubhav_aeron@intuit.com>
Signed-off-by: Anubhav Aeron <anubhav_aeron@intuit.com>
Signed-off-by: Anubhav Aeron <anubhav_aeron@intuit.com>
@nirvanagit
Copy link
Collaborator Author

The change looks good. Should we add a unit with mock controller one to deleting an existing resource and another to delete a non existing one?

done.

Copy link
Contributor

@aattuluri aattuluri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nirvanagit nirvanagit merged commit 05fc3db into master Aug 29, 2022
@nirvanagit nirvanagit deleted the vs-deletion branch August 29, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants