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

Set the client namespace on uninstall #82

Merged
merged 1 commit into from
Mar 15, 2021

Conversation

Itxaka
Copy link
Contributor

@Itxaka Itxaka commented Mar 11, 2021

We are missing reading from the chart annotations as well,
or maybe thats not gonna be implemented?

We are missing this piece to be able to clean up the resources properly but probably missing obtaining the namespace from annotattions which is quite bigger....and would probably benefit from extraction some generic methods...

Partial fix: #77

Signed-off-by: Itxaka [email protected]

We are missing reading from the chart annotations as well,
or maybe thats not gonna be implemented?

Signed-off-by: Itxaka <[email protected]>
@Itxaka Itxaka added bug Something isn't working WIP This item is Work in Progress, do not merge! labels Mar 11, 2021
@viccuad
Copy link
Member

viccuad commented Mar 12, 2021

Looks good! I think this is it. And I don't think there's any need to look for annotations; the uninstall command works on releases on the store, not on charts, so there's no annotations to be looked at!

@viccuad
Copy link
Member

viccuad commented Mar 12, 2021

I think we should add some tests to catch the problems (maybe out of this PR). Adding tests to cmd/hypper/uninstall_test.go doesn't give us anything, as those check for command output. I thought adding pkg/action/uninstall_test.go shouldn't be needed, but as we are using our own Config instead of the one inside of helm/pkg/action/install.go, then we should write tests testing our Config.

@Itxaka Itxaka removed the bug Something isn't working label Mar 15, 2021
@Itxaka
Copy link
Contributor Author

Itxaka commented Mar 15, 2021

I think we should add some tests to catch the problems (maybe out of this PR). Adding tests to cmd/hypper/uninstall_test.go doesn't give us anything, as those check for command output. I thought adding pkg/action/uninstall_test.go shouldn't be needed, but as we are using our own Config instead of the one inside of helm/pkg/action/install.go, then we should write tests testing our Config.

Do we really? Like, are we doing something any special that setNamespace (which should be tested of course)? The rest is basically the same as helm and should be covered by helm testing on their side no?

@Itxaka Itxaka merged commit 9508eff into rancher-sandbox:main Mar 15, 2021
@Itxaka Itxaka deleted the fix_uninstall_namespace_flag branch March 15, 2021 08:54
@Itxaka Itxaka added bug Something isn't working and removed WIP This item is Work in Progress, do not merge! labels Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hypper uninstall not working correctly
2 participants