Add support for `lokoctl component delete` #268

surajssd · 2020-04-06T14:42:32Z

NOTE: For detailed information, see changes per commit.

Fixes #67

pkg/components/util/doc.go

Makefile

cli/cmd/doc.go

cli/cmd/component-delete.go

pkg/components/util/delete.go

invidian · 2020-04-06T15:11:39Z

pkg/components/util/delete.go

+}
+
+// AggregateErrors conflates multiple errors stored in a string form and returns a single error.
+func AggregateErrors(errs []string) error {


Why not use hcl.Diagnostics instead?

I think hcl.Diagnostics is not very helpful to the end user because it does not list all the errors and oly gives output as follows:

FATA[0016] <nil>: uninstall: Release not loaded: openebs-operator: release: not found; , and 4 other diagnostic(s) args="[]" command="lokoctl component delete"

This is not very helpful. As a user I want to know all the errors that have occurred.

@surajssd you need to iterate over it and print each one in desired format to make it usable for the user 😉

Then the API is not very user friendly. Because this function AggregateErrors clubs all the errors one on each line and the output looks like following:

$ lokoctl component delete --delete-namespace Do you want to delete components: "openebs-operator,openebs-storage-class,prometheus-operator,contour,metallb,cert-manager,httpbin"? [type "yes" to continue]: yes Deleting component 'openebs-operator'... Deleting component 'openebs-storage-class'... Deleting component 'prometheus-operator'... Deleting component 'contour'... Deleting component 'metallb'... Deleting component 'cert-manager'... Deleting component 'httpbin'... FATA[0008] uninstall: Release not loaded: openebs-operator: release: not found uninstall: Release not loaded: openebs-storage-class: release: not found uninstall: Release not loaded: prometheus-operator: release: not found namespaces "monitoring" not found uninstall: Release not loaded: contour: release: not found namespaces "projectcontour" not found uninstall: Release not loaded: metallb: release: not found namespaces "metallb-system" not found uninstall: Release not loaded: cert-manager: release: not found namespaces "cert-manager" not found uninstall: Release not loaded: httpbin: release: not found namespaces "httpbin" not found args="[]" command="lokoctl component delete"

¯\(ツ)/¯

I think we don't need this function - see #268 (comment).

cli/cmd/component-delete.go

ipochi · 2020-04-07T09:23:30Z

@surajssd we will need to update the cli documentation.
@BrainBlasted implemented something from Cobra that generates the cli documentation.

surajssd · 2020-04-07T09:30:11Z

Another instance of golangci-lint failing in CI and not locally. It failed in CI with following error:

...
# Note: Make sure that you run `git config diff.noprefix false` in this repo
# See this issue for more details: https://github.com/golangci/golangci-lint/issues/948
golangci-lint run --enable-all --disable=godox,gochecknoglobals --max-same-issues=0 --max-issues-per-linter=0 --build-tags "aws,packet,e2e,disruptivee2e,poste2e" --new-from-rev=$(git merge-base $(cat .git/resource/base_sha 2>/dev/null || echo "master") HEAD) --modules-download-mode=vendor --timeout=5m --exclude-use-default=false ./...
pkg/components/util/delete.go:78:7: mnd: Magic number: 1, in <case> detected (gomnd)
	case 1:
	     ^
make: *** [Makefile:66: lint] Error 1

But does not fail locally. My version of the tool is golangci-lint has version 1.24.0 built from 6fd4383 on 2020-03-15T11:38:02Z

invidian · 2020-04-07T12:59:52Z

But does not fail locally. My version of the tool is golangci-lint has version 1.24.0 built from 6fd4383 on 2020-03-15T11:38:02Z

On CI we use 1.13.8, as 1.14.0 has issues with memory usage.

Another instance of golangci-lint failing in CI and not locally. It failed in CI with following error:

I was hitting the same, but I'm running 1.14.0 locally too.

@surajssd what about running make lint-docker?

invidian · 2020-04-08T16:16:03Z

@surajssd I pulled some extra reviewers, as I'm not convinced with AggregateErrors.

cli/cmd/component-delete.go

pkg/components/util/delete.go

joaquimrocha

My 2 cent.

cli/cmd/component-delete.go

iaguis

Some comments.

cli/cmd/component-delete.go

pkg/components/util/delete.go

surajssd · 2020-04-20T06:54:54Z

The PR is updated, PTAL @invidian @iaguis @johananl

invidian

It would also be good to add some tests for uninstalling the components, as we have no unit tests for this either.

cli/cmd/component-delete.go

invidian · 2020-04-20T07:51:04Z

cli/cmd/component-delete.go

+		contextLogger.Fatal(diags)
+	}
+
+	componentsToDelete := make([]string, len(args))


Why do we need to use make and copy here? Why can't we just do:

d := args if len(d) == 0 { for _, c := range lokoCfg.RootConfig.Components { d = append(d, c.Name) } }

I think this code is much simpler and less confusing for people starting with Go.

IMO it idiomatic to make and copy. Current code is just 2 lines.

And for someone starting with golang they will learn new things, won't they?

I just think my approach is simpler and is something you usually see in Go code, so I'm asking, if there is some specific reason you wrote it this way?

cli/cmd/component-delete.go

invidian · 2020-04-20T08:36:18Z

cli/cmd/component-delete.go

+
+	resp, err := uninstall.Run(name)
+	// ignore the err when we have deleted the release already or it does not exist for some reason
+	if err != nil && !strings.Contains(err.Error(), "uninstall: Release not loaded") {


Maybe we could check if the release exists before trying to uninstall it to avoid doing string comparison on errors. This seems rather fragile.

done, but even when we check if the installation/release exists. It all boils down on what assumptions we are willing to trust more?

IMO it's better to check if the release exists, as this is the same what helm is doing and it's a GET operation, which is OK if it fails, where DELETE is something which should not fail normally. Also with GET there is specific error type as far as I remember.

invidian

Sorry @surajssd, that I didn't look thoroughly enough before, I put few more comments on things I'd like to clarify.

cli/cmd/component-delete.go

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

This commit adds `Name` field in the struct `Metadata`. This helps identify what that component is. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

This commit adds support of deleting the installed the component. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

All the changes were incorporated :-)

surajssd requested review from invidian and ipochi as code owners April 6, 2020 14:42

invidian suggested changes Apr 6, 2020

View reviewed changes

surajssd force-pushed the surajssd/uninstall-component branch from 6395159 to ae85bf0 Compare April 7, 2020 09:16

surajssd requested a review from johananl as a code owner April 7, 2020 09:20

surajssd force-pushed the surajssd/uninstall-component branch 3 times, most recently from 2bc4e8b to f2db453 Compare April 7, 2020 10:43

invidian mentioned this pull request Apr 7, 2020

[WIP] Add cli subcommand for component uninstall #74

Closed

surajssd requested a review from invidian April 8, 2020 16:05

invidian requested review from iaguis and rata April 8, 2020 16:15

johananl reviewed Apr 14, 2020

View reviewed changes

cli/cmd/component-delete.go Outdated Show resolved Hide resolved

pkg/components/util/delete.go Outdated Show resolved Hide resolved

pkg/components/util/delete.go Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/uninstall-component branch from f2db453 to 4658ccc Compare April 15, 2020 14:21

joaquimrocha previously requested changes Apr 15, 2020

View reviewed changes

cli/cmd/component-delete.go Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/uninstall-component branch 4 times, most recently from 7932019 to 67e6cf7 Compare April 17, 2020 10:29

iaguis reviewed Apr 17, 2020

View reviewed changes

cli/cmd/component-delete.go Outdated Show resolved Hide resolved

pkg/components/util/delete.go Outdated Show resolved Hide resolved

pkg/components/util/delete.go Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/uninstall-component branch from 67e6cf7 to 5aa9a9d Compare April 17, 2020 13:37

surajssd requested review from iaguis, johananl and joaquimrocha April 17, 2020 13:38

surajssd force-pushed the surajssd/uninstall-component branch from 5aa9a9d to e7842e2 Compare April 17, 2020 13:47

invidian suggested changes Apr 20, 2020

View reviewed changes

surajssd force-pushed the surajssd/uninstall-component branch from e7842e2 to d03568a Compare April 20, 2020 12:55

invidian reviewed Apr 20, 2020

View reviewed changes

cli/cmd/component-delete.go Outdated Show resolved Hide resolved

cli/cmd/component-delete.go Outdated Show resolved Hide resolved

cli/cmd/component-delete.go Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/uninstall-component branch from f780b7c to 749154a Compare April 20, 2020 14:18

surajssd requested a review from invidian April 21, 2020 11:37

invidian previously approved these changes Apr 21, 2020

View reviewed changes

surajssd dismissed invidian’s stale review via a0ca390 April 21, 2020 14:55

surajssd force-pushed the surajssd/uninstall-component branch 7 times, most recently from 958cb7b to a764fbb Compare April 22, 2020 12:32

surajssd added 5 commits April 22, 2020 18:37

go fmt: Fix import formatting

6e7de9d

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

pkg/components/util: Add doc.go

3d5c1ac

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

component: Refactor to add name in metadata

123c568

This commit adds `Name` field in the struct `Metadata`. This helps identify what that component is. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

cmd: Add lokoctl component delete

1a63169

This commit adds support of deleting the installed the component. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

cli/cmd: Add package level comment

f3cf5cf

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd force-pushed the surajssd/uninstall-component branch 3 times, most recently from 77962b4 to bf16a66 Compare April 22, 2020 13:37

docs: Add lokoctl component delete

7b2f936

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd force-pushed the surajssd/uninstall-component branch from bf16a66 to 7b2f936 Compare April 22, 2020 14:11

surajssd requested a review from invidian April 22, 2020 15:03

invidian approved these changes Apr 23, 2020

View reviewed changes

surajssd merged commit a6c6db6 into master Apr 23, 2020

surajssd deleted the surajssd/uninstall-component branch April 23, 2020 09:22

surajssd mentioned this pull request Oct 5, 2020

Should uninstalling not configured component return error? #1019

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `lokoctl component delete` #268

Add support for `lokoctl component delete` #268

surajssd commented Apr 6, 2020 •

edited

Loading

invidian Apr 6, 2020

surajssd Apr 7, 2020

invidian Apr 7, 2020

surajssd Apr 7, 2020 •

edited

Loading

invidian Apr 8, 2020 •

edited

Loading

johananl Apr 14, 2020

ipochi commented Apr 7, 2020

surajssd commented Apr 7, 2020

invidian commented Apr 7, 2020

invidian commented Apr 8, 2020

joaquimrocha left a comment

iaguis left a comment

surajssd commented Apr 20, 2020

invidian left a comment

invidian Apr 20, 2020

surajssd Apr 20, 2020

surajssd Apr 20, 2020

invidian Apr 20, 2020

invidian Apr 20, 2020

surajssd Apr 20, 2020

invidian Apr 20, 2020

invidian left a comment

Add support for lokoctl component delete #268

Add support for lokoctl component delete #268

Conversation

surajssd commented Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

invidian Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipochi commented Apr 7, 2020

surajssd commented Apr 7, 2020

invidian commented Apr 7, 2020

invidian commented Apr 8, 2020

joaquimrocha left a comment

Choose a reason for hiding this comment

iaguis left a comment

Choose a reason for hiding this comment

surajssd commented Apr 20, 2020

invidian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

Add support for `lokoctl component delete` #268

Add support for `lokoctl component delete` #268

surajssd commented Apr 6, 2020 •

edited

Loading

surajssd Apr 7, 2020 •

edited

Loading

invidian Apr 8, 2020 •

edited

Loading