feat(*): add installation delete options/flags to relevant commands #1189

vdice · 2020-07-31T22:07:05Z

What does this change

Adds logic to enable deleting an installation, either via porter uninstall or porter installation delete (as designed in Do we need a purge? #1163)

What issue does it fix

Closes #1163

Notes for the reviewer

Do we want any additional docs besides the auto-gen'd CLI docs?
Suggestions for simplifying logic around uninstall +/- delete +/- force-delete welcome.
Note how we are swallowing errors around the execution of the uninstall action if force-delete is true -- do we like this approach? Or should we continue to return any/all execution errors after we force delete the installation?

Checklist

Unit Tests
Documentation
Schema (porter.yaml) N/A

If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

carolynvs

I wasn't able to finish reviewing but here are some initial comments

cmd/porter/bundle.go

cmd/porter/installations.go

carolynvs · 2020-08-04T18:09:36Z

pkg/cnab/provider/helpers.go

@@ -45,6 +45,8 @@ func (t *TestRuntime) LoadBundle(bundleFile string) (bundle.Bundle, error) {
 }

 func (t *TestRuntime) Execute(args ActionArguments) error {
-	args.Driver = debugDriver
+	if args.Driver == "" {


pkg/context/helpers.go

pkg/porter/delete.go

pkg/porter/delete_test.go

carolynvs

Okay! Reviewed all the files now

carolynvs · 2020-08-04T20:37:03Z

pkg/porter/dependencies.go

+
+		if depArgs.Action == claim.ActionUninstall {
+			if e.parentOpts.Delete && !e.parentOpts.ForceDelete {
+				executeErrs = multierror.Append(executeErrs, fmt.Errorf("not deleting installation %s as uninstall was not successful; use --force-delete to override", depArgs.Installation))


Can you update this error message to match the other as well that you just updated?

carolynvs · 2020-08-04T20:39:47Z

pkg/porter/uninstall.go

 	fmt.Fprintf(p.Out, "uninstalling %s...\n", opts.Name)
 	err = p.CNAB.Execute(opts.ToActionArgs(deperator))
 	if err != nil {
+		uninstallErrs = multierror.Append(uninstallErrs, err)
+		if strings.Contains(err.Error(), claim.ErrInstallationNotFound.Error()) {


Can you add a comment on why this special handling for ErrInstallationNotFound is needed?

carolynvs · 2020-08-04T20:40:33Z

pkg/porter/uninstall.go

+		}
+
+		if opts.Delete && !opts.ForceDelete {
+			uninstallErrs = multierror.Append(uninstallErrs, fmt.Errorf("not deleting installation %s as uninstall was not successful; use --force-delete to override", opts.Name))


Sorry looks like that error message I had you update was used here as well.

carolynvs · 2020-08-04T20:43:04Z

pkg/porter/uninstall.go

+		if !opts.ForceDelete {
+			return uninstallErrs
+		}
+		// else, we swallow uninstallErrs as opts.ForceDelete is true


I think it would be helpful to no matter what, print out the errors even on --force-delete. We just will have a 0 exit code for --force-delete even when there are errors. That way you know what happened.

carolynvs · 2020-08-04T20:48:51Z

pkg/porter/uninstall.go

 	}

 	// TODO: See https://github.com/deislabs/porter/issues/465 for flag to allow keeping around the dependencies
-	return deperator.Execute()
+	err = deperator.Execute()
+	if err != nil {


Right now it's confusing how the ForceDelete/Delete is handled by both this function and the Deperator, and then they have all sorts of conditionals for handling the error, printing that warning about using --force, etc. I am concerned that it's going to be difficult to maintain without introducing bugs or fixing a bug accidentally in just one place (and forgetting to fix it in the dependencies.go for example).

🤔 Looking at it in a github code review it's hard to say "just change X" to simplify it. But I do wish there was a way to do so. Maybe we can take a second pass at it and see if there's a way to either extract the logic for dealing with it, or consolidate the logic for checking and printing the messages? I'm not sure what to suggest which is why I think trying again in a follow-up or whenever we next feel clever or inspired is fine.

One idea is to have a function takes in the flags (delete, force delete and the installation status) and it handles deciding if we should delete the installation and printing the warning. Then at least we could reuse that in 2+ places?

You're right, this area warrants further thought. Thank you for the initial suggestions. I'll spend some more time here and hopefully have an update w/ less fragility and duplication.

(Still working on this one...)

Ok, refactored for a bit of simplification... or at least some shared logic. Not sure if better, but perhaps it might generate some further ideas: dd5dd04

carolynvs · 2020-08-04T20:50:40Z

tests/dependencies_test.go

@@ -31,6 +31,8 @@ func TestDependenciesLifecycle(t *testing.T) {
 	invokeWordpressBundle(p, namespace)

 	uninstallWordpressBundle(p, namespace)
+
+	noReallyUninstallWordpressBundle(p, namespace)


Oh, I just realized, should I just place this uninstall+delete logic into cleanupWordpressBundle? Or keep it separate?

(I updated cleanupWordpressBundle w/ uninstall+delete logic in 6af718a)

carolynvs · 2020-08-04T20:53:44Z

tests/uninstall_test.go

+		installationExists bool
+		wantError          string
+	}{
+		{"not yet installed", false, false, false, false, false, "1 error occurred:\n\t* could not load installation HELLO: Installation does not exist\n\n"},


I suggest test cases like this, which are really long, from setting every field without using the field name (which is quite hard to read without the support of an editor) to using the field names and only setting the ones that are non-default values,

{name: "not yet installed", wantError: "1 error occurred..."},

carolynvs · 2020-08-10T14:10:51Z

tests/dependencies_test.go

+func cleanupWordpressBundle(p *porter.TestPorter, namespace string) {
+	uninstallOptions := porter.UninstallOptions{}
+	uninstallOptions.CredentialIdentifiers = []string{"ci"}
+	uninstallOptions.Delete = true


carolynvs · 2020-08-18T11:48:58Z

pkg/porter/lifecycle.go

@@ -9,6 +9,7 @@ type BundleLifecycleOpts struct {
 	sharedOptions
 	BundlePullOptions
 	AllowAccessToDockerHost bool
+	UninstallDeleteOptions


This isn't the right place for these options as it applies to a bunch of commands: install, upgrade, invoke, etc. Can you move it to UninstallOptions as I think these flags only apply to the uninstall command, right?

carolynvs · 2020-08-18T12:08:56Z

pkg/porter/dependencies.go


-		dep.outputs = outputs
+	if e.parentOpts.shouldDelete() {


I think you put the UninstallDeleteOptions on the generic BundleLifecycleOptions so that you could make this check here, right?

Instead, since I'm asking further down to move that to the UninstallOptions type would be to do a type check

if uninstOpts, ok := e.parentOpts.(UninstallOptions); ok { ... }

Right now parentOpts is just a BundleLifecycleOptions but we could make it an interface (pseudo code, not tested, but something like below), so that we could cast and get at the original options as needed while still having access to the other data that we were using on the parentOpts the rest of the time.

What do you think?

type BundleAction interface { // Implemented for free because of the embedded BundleLifecycleOpts ToActionArgs() // Let us grab and get direct access to the bundle opts fields like we need in some funcs GetBundleLifecycleOptions() }

Great suggestion! Thank you for providing the blueprint on this approach. Here's what I've updated: 4b2b647

How does it look?

carolynvs · 2020-08-18T12:09:59Z

pkg/porter/uninstall.go

+	return opts.Delete && !opts.ForceDelete
+}
+
+func (opts *UninstallDeleteOptions) handleUninstallErrs(out io.Writer, err error) error {


Signed-off-by: Vaughn Dice <vadice@microsoft.com>

Co-authored-by: Carolyn Van Slyck <me@carolynvanslyck.com>

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

…cuting dependencies Signed-off-by: Vaughn Dice <vadice@microsoft.com>

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs

Looks good, just had a question that I'm not sure matters yet or not

carolynvs · 2020-08-18T19:32:57Z

pkg/porter/dependencies.go

 		if executeErrs != nil {
 			return executeErrs
 		}
 	}

-	if e.parentOpts.shouldDelete() {
+	if uninstallOpts.shouldDelete() {


If uninstallOpts is an empty struct (because parentOpts was some other action), what does this call do?

I think all methods on an empty struct (when action not uninstall) return the uninitialized/default values for the struct fields... so mostly boolean false values, which is what we'd want here. The tests seem to agree.

However, I can see this approach/code being fragile... even if the assumptions are true. What do we think? Should we refactor the areas where we rely on uninstallOpts to run only if we are sure the struct is actually that type (and not empty)?

I think it would get pretty gnarly to conditionally call those, so the empty struct is fine. I was just double checking. Maybe let's just add a comment about if it's not uninstall, it's a no-op handler?

carolynvs · 2020-08-18T19:33:42Z

pkg/porter/lifecycle.go

+// when the implementation contains further, action-specific options beyond
+// the stock BundleLifecycleOptions.
+type BundleAction interface {
+	ToActionArgs(*dependencyExecutioner) cnabprovider.ActionArguments


Looks like this isn't used anywhere? I thought it may be used depending on how the implementation worked out but guess not.

it is used here: https://github.com/deislabs/porter/blob/4b2b64746636e6997ee1f3dae8c87953c8b0cddc/pkg/porter/dependencies.go#L86

But, to your point, it could just as easily be removed from this interface and exist as it once was (a method on BundleLifecycleOpts), since at this time we don't see a need to have BundleAction implementations overriding its behavior. Shall I change this back and remove the method from the interface?

Oh, thanks I obviously missed that. Either way is fine, up to you!

Ok, removed from interface 👍

carolynvs · 2020-08-18T19:34:28Z

pkg/porter/lifecycle.go

@@ -28,8 +39,12 @@ func (o *BundleLifecycleOpts) Validate(args []string, porter *Porter) error {
 	return nil
 }

+func (o BundleLifecycleOpts) GetBundleLifecycleOptions() BundleLifecycleOpts {


Brilliant! I wouldn't have thought to implement this here, and would have put it on each of the actions but this is sneakier. 💯

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs

Thanks this is going to be super helpful for cleaning up my dev machine! 😂

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

docs(cli): missed doc updates from PR #1189

vdice requested review from carolynvs and jeremyrickard as code owners July 31, 2020 22:07

carolynvs added this to In Progress in Porter and Mixins [OLD] via automation Aug 4, 2020

carolynvs reviewed Aug 4, 2020

View reviewed changes

vdice force-pushed the feat/the-life-changing-magic-of-tidying-up-installations branch from 42f8439 to fbf316c Compare August 4, 2020 19:16

carolynvs reviewed Aug 4, 2020

View reviewed changes

vdice force-pushed the feat/the-life-changing-magic-of-tidying-up-installations branch from ccebc29 to 29d0a32 Compare August 6, 2020 16:10

carolynvs self-assigned this Aug 6, 2020

carolynvs reviewed Aug 10, 2020

View reviewed changes

carolynvs requested changes Aug 18, 2020

View reviewed changes

vdice and others added 8 commits August 18, 2020 09:50

feat(*): add installation delete options/flags to relevant commands

30e8fb1

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

Apply suggestions from code review

1e0c21e

Co-authored-by: Carolyn Van Slyck <me@carolynvanslyck.com>

incorporate add'l review feedback

222b196

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

add'l review feedback

ba43169

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

refactor the parent/dep uninstall scenarios

b77e8c2

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

update dependencies_test.go

6c64917

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

use BundleAction interface to enable action-specific options when exe…

4b2b647

…cuting dependencies Signed-off-by: Vaughn Dice <vadice@microsoft.com>

ref(tests/dependencies_test.go): set mysql dep namespace param as well

353dfd5

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

vdice force-pushed the feat/the-life-changing-magic-of-tidying-up-installations branch from 6af718a to 353dfd5 Compare August 18, 2020 19:19

fix cmd example spacing

7f5b60c

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs reviewed Aug 18, 2020

View reviewed changes

vdice added 2 commits August 18, 2020 15:46

add comments around uninstallOpts when empty

27f00be

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

remove ToActionArgs from BundleAction interface

97a31c4

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs approved these changes Aug 19, 2020

View reviewed changes

carolynvs merged commit c525954 into getporter:main Aug 19, 2020

Porter and Mixins [OLD] automation moved this from In Progress to Done Aug 19, 2020

vdice deleted the feat/the-life-changing-magic-of-tidying-up-installations branch August 19, 2020 14:48

vdice added a commit to vdice/porter that referenced this pull request Aug 19, 2020

docs(cli): missed doc updates from PR getporter#1189

c2a09d6

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

vdice mentioned this pull request Aug 19, 2020

docs(cli): missed doc updates from PR #1189 #1219

Merged

3 tasks

carolynvs added a commit that referenced this pull request Aug 20, 2020

Merge pull request #1219 from vdice/docs/oops

81b1d47

docs(cli): missed doc updates from PR #1189

vdice removed this from Done in Porter and Mixins [OLD] Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(*): add installation delete options/flags to relevant commands #1189

feat(*): add installation delete options/flags to relevant commands #1189

vdice commented Jul 31, 2020

carolynvs left a comment

carolynvs Aug 4, 2020

carolynvs left a comment

carolynvs Aug 4, 2020

carolynvs Aug 4, 2020

carolynvs Aug 4, 2020

carolynvs Aug 4, 2020

carolynvs Aug 4, 2020

vdice Aug 4, 2020

vdice Aug 6, 2020

vdice Aug 6, 2020

carolynvs Aug 4, 2020

vdice Aug 6, 2020

vdice Aug 7, 2020

carolynvs Aug 4, 2020

carolynvs Aug 10, 2020

carolynvs Aug 18, 2020

carolynvs Aug 18, 2020

vdice Aug 18, 2020

carolynvs Aug 18, 2020

carolynvs left a comment

carolynvs Aug 18, 2020

vdice Aug 18, 2020 •

edited

carolynvs Aug 18, 2020

carolynvs Aug 18, 2020

vdice Aug 18, 2020

carolynvs Aug 18, 2020

vdice Aug 18, 2020 •

edited

carolynvs Aug 18, 2020

carolynvs left a comment

feat(*): add installation delete options/flags to relevant commands #1189

feat(*): add installation delete options/flags to relevant commands #1189

Conversation

vdice commented Jul 31, 2020

What does this change

What issue does it fix

Notes for the reviewer

Checklist

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdice Aug 18, 2020 • edited

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

vdice Aug 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

vdice Aug 18, 2020 •

edited

vdice Aug 18, 2020 •

edited