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 delete command #87

Merged
merged 9 commits into from
Dec 23, 2020
Merged

feat: cli delete command #87

merged 9 commits into from
Dec 23, 2020

Conversation

Vafilor
Copy link
Contributor

@Vafilor Vafilor commented Nov 26, 2020

What this PR does:

Adds a basic delete command.

Which issue(s) this PR fixes:

Fixes onepanelio/onepanel#270

Special notes for your reviewer:

This does not wait to see if all resources have been deleted, so it may take the cluster a few seconds to delete everything.

Use in conjunction with: onepanelio/manifests#102

@Vafilor Vafilor added this to the v0.16.0 milestone Nov 26, 2020
@aleksandrmelnikov
Copy link
Contributor

aleksandrmelnikov commented Nov 30, 2020

@Vafilor Wanted to bring this up for consideration.
I deployed with CLI, using "default" for namespace.
During clean-up:

Unable to delete: error when deleting ".onepanel/kubernetes.yaml": namespaces "default" is forbidden: this namespace may not be deleted

Since it wasn't deleted, it's didn't impact the k8s cluster.
I see we use application-system in our manifests, so was this supposed to be cleaned-up as a namespace as well?

  • If so, that error may have stopped application-system namespace from being deleted.

Not sure if we should point this out via code, or ignore it via code, or not worry about this error.

@Vafilor
Copy link
Contributor Author

Vafilor commented Dec 1, 2020

@Vafilor Wanted to bring this up for consideration.
I deployed with CLI, using "default" for namespace.
During clean-up:

Unable to delete: error when deleting ".onepanel/kubernetes.yaml": namespaces "default" is forbidden: this namespace may not be deleted

Since it wasn't deleted, it's didn't impact the k8s cluster.
I see we use application-system in our manifests, so was this supposed to be cleaned-up as a namespace as well?

  • If so, that error may have stopped application-system namespace from being deleted.

Not sure if we should point this out via code, or ignore it via code, or not worry about this error.

@aleksandrmelnikov That's a good point, I didn't consider that case.
It'd be easiest if we didn't allow the default namespace of course. But that might be inconvenient.

What do you think @rushtehrani? Should we not allow default for now, just rework logic so default is allowed?

@rushtehrani
Copy link
Contributor

What do you think @rushtehrani? Should we not allow default for now, just rework logic so default is allowed?

@Vafilor I am good with disallowing default and updating CLI and manifest default and comments accordingly.

@Vafilor
Copy link
Contributor Author

Vafilor commented Dec 1, 2020

What do you think @rushtehrani? Should we not allow default for now, just rework logic so default is allowed?

@Vafilor I am good with disallowing default and updating CLI and manifest default and comments accordingly.

Sounds good. How about sample for the new "default" namespace?

@rushtehrani
Copy link
Contributor

Sounds good. How about sample for the new "default" namespace?

I think something like my-project would make more sense. Ideally we make it <namespace> and show an error if they left it as a placeholder.

@Vafilor
Copy link
Contributor Author

Vafilor commented Dec 1, 2020

Sounds good. How about sample for the new "default" namespace?

I think something like my-project would make more sense. Ideally we make it <namespace> and show an error if they left it as a placeholder.

<namespace> with error detection is doable. I'll add this to this PR.

@Vafilor Vafilor marked this pull request as draft December 1, 2020 04:18
@Vafilor Vafilor marked this pull request as ready for review December 4, 2020 22:33
@@ -12,6 +12,7 @@ func DeploymentStatus(yamlFile *DynamicYaml) (ready bool, err error) {
namespacesToCheck["application-system"] = true
namespacesToCheck["onepanel"] = true
namespacesToCheck["istio-system"] = true
namespacesToCheck["default"] = true
Copy link
Contributor

Choose a reason for hiding this comment

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

@Vafilor added default to reserved ns list here

@rushtehrani rushtehrani modified the milestones: v0.16.0, v0.17.0 Dec 11, 2020
@Vafilor Vafilor added the kind/enhancement New feature or request label Dec 23, 2020
@Vafilor Vafilor self-assigned this Dec 23, 2020
@Vafilor Vafilor merged commit f1a6a55 into master Dec 23, 2020
@Vafilor Vafilor deleted the feat/onepanel.delete branch December 23, 2020 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opctl delete
3 participants