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

feat(cli): add ignore-not-found flag #1458

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

mariuskimmina
Copy link
Contributor

What issue type does this pull request address? (keep at least one, remove the others)
/kind feature

What does this pull request do? Which issues does it resolve? (use resolves #<issue_number> if possible)
resolves #771

Please provide a short message that should be published in the vcluster release notes
Added --ignore-not-found flag to prevent errors when the virtual cluster or the namespace was not found

What else do we need to know?
This was partially based on #989 which got closed to due to inactivity

if !errors.As(err, &errorNotFound) {
return err
}
vclusterFound = false
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to keep going here? Can we not just return nil and be done with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that but then a command like

vcluster delete my-cluster --namespace my-namespace --delete-namespace --ignore-not-found

would not delete the namespace if the namespace exists but the vcluster does not.
I wanted to make sure that the namespace would also get cleaned up in such a case

Copy link
Member

Choose a reason for hiding this comment

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

I think its okay to not delete the namespace if the vcluster doesn't exist, since that operation clearly was either aborted or the user messed somehow differently with the vcluster deployment.

Copy link
Member

Choose a reason for hiding this comment

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

If this should become a problem, we could add that later then as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so keeping it as simple as possible for now, then I agree we can just return nil here and be done with it. I changed the PR accordingly

@FabianKramm
Copy link
Member

@mariuskimmina can you fix the lint issues?

@mariuskimmina
Copy link
Contributor Author

@FabianKramm fixed the linter issue and also squashed the commits

@FabianKramm FabianKramm merged commit 962436f into loft-sh:main Jan 24, 2024
59 checks passed
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.

vcluster delete --ignore-not-found just like kubectl
2 participants