Fix "consul-k8s install fails: now can't re-install or upgrade"

[t-eckert]

Fixes #1005

Changes proposed in this PR:

• Testing
  • Extend HelmCluster and CLICluster to accept arguments on Create, Update, and Delete actions.
  • Allow tests to set the namespace used for CLI acceptance tests.
  • Add a scenario test to exercise upgrading Consul after a failed install.
  • Add a scenario test to exercise upgrading Consul installed in the default namespace.
• Bug fix
  • Fix an issue in the upgrade case where the cluster was being found by namespace instead of its name.

How I've tested this PR:

Both using scenario tests and by debugging the upgrade command when an install was present in the default namespace.

How I expect reviewers to test this PR:

The scenario tests may be run from the acceptance directory with

go test -timeout 15m github.com/hashicorp/consul-k8s/acceptance/tests/cli -v

Checklist:

• Tests added
• CHANGELOG entry added

  HashiCorp engineers only, community PRs should not add a changelog entry.
  Entries should use present tense (e.g. Add support for...)

[jmurret]

Just starting to look through this. There are a lot of places where you are looking for args and if they don't exist you use default args. Would it be easier or more correct to merge the provided args onto the default args and use that merged map? (I am not saying it is easier or more correct, but just that with default values and supplied values, that's the common pattern I see.)

[t-eckert]

Just starting to look through this. There are a lot of places where you are looking for args and if they don't exist you use default args. Would it be easier or more correct to merge the provided args onto the default args and use that merged map? (I am not saying it is easier or more correct, but just that with default values and supplied values, that's the common pattern I see.)

Interesting question. I can definitely look at that. I can give that a shot with an args map[string]string being passed in and get your opinion on that.

[jmurret]

ok, i said map, but now that you mention it and i look again you are setting the extra args for each function and its an array. I don't think you need to bother changing that structure. What I was getting at is you may want to take default args and append the passed in args to get the working set of args and that would represent better how default and user specified options are merged together.
You can do an append and if a passed in arg was already in the default set, it would result in two of the same arg. Maybe that does not cause issues..but you could also quickly check if it exists in the default set of args and then skip appending it.
anyway, i would not spend a lot of time on this. It was just more of an inquiry or a nit than anything. Since this is infrastructure code for tests, if it works, we can also just move ahead and fix anything that comes up in the future. I think you could try out the contains/append with the default and passed in args pretty quickly tho, so if you do anything, you may time box that and then call it good.

[t-eckert]

Hm, if I do that, I will actually use a map and then flatten it because some flags take a value and others do not. I can map string to *string and implement this kind of merging without having to hard-code logic of which args take values and which do not.

[t-eckert]

So, when I did that I remembered why I went with ...string as an argument. Otherwise, I have to change the call to these methods in a few dozen or so places to pass in an empty map.

[jmurret]

🎉 🙏 awesome find and fix! Minor nit on file name and wont' block on the ongoing conversation about merging args since everything works and don't want to draw this out too much.

[ishustava]

Nice fix, Thomas!

I'm leaving some comments and questions in-line.

[ishustava]

Looks good!