cli: support install command #713

ndhanushkodi · 2021-09-03T01:36:08Z

(This description is lightly edited from the original PR Saad created: https://github.com/hashicorp/consul-k8s-cli/pull/2 We migrated the PR here and migrated some of the review comments as well)

Changes proposed in this PR:

consul-k8s install installs the latest Helm chart onto your Kubernetes cluster after performing a few verification steps. Note that once the CLI is moved into the monorepo, the version of the Helm chart deployed will match the version in the repository.
For details about flags and their intended behavior visit the RFC, linked here.
common/flag is copied from Waypoint’s internal/pkg subdirectory here. It is a thin layer over the standard Go flag package that provides features such as aliasing and global flags. It was created specifically for mitchellh/cli. See also this Slack thread for the decision to use it.
common/terminal package is adapted from the Waypoint Plugin SDK and excludes the Status interface and Step interface as described in doc.go. It allows us to have a consistent UI output with style.
common/base.go describes a base command that can be embedded into all commands, and pass in common functionality like UI and ctx. This pattern was adapted from Waypoint.
~~common/utils.go contains a function WithInterrupt() that we take from signalcontext.go in Waypoint's internal/pkg.~~

How I've tested this PR:

Created rather simple unit tests that validate two different helper functions inside of install.go. The first helper function we test is validateFlags() which performs a lot of the basic sanity checks before starting the install. The second helper is validLabel() which makes sure that the namespace the user passes is legal, in that if follows RFC 1123 label convention.
In addition to the actual unit tests, a lot of manual ad-hoc testing took place. Follow the instructions below to build the CLI and test the various flags, especially the -set ones, and see how they interact with one another.
Note that more rigorous acceptance tests are soon to come after monorepo is finished. These tests will actually run the installation through and validate it worked properly.

How I expect reviewers to test this PR:

Try it out :
git clone https://github.com/hashicorp/consul-k8s-cli.git
cd consul-k8s-cli
go build -o consul-k8s
consul-k8s install
Try this with different flags and various configurations. Note that if you are running on minikube, it is recommended to install with -preset=demo. That way server.replicas is set to 1. I also found it helpful to specify the namespace when manually testing via -namespace=not-peppertrout.
Code review :
Pay careful attention to install.go, specifically the pre-install checks. If you see anything you believe should be pulled out into an individual function and/or tested more thoroughly please comment!

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...)

Co-authored-by: Saad Jamal saad@hashicorp.com
Co-authored-by: Nitya Dhanushkodi nitya@hashicorp.com

ndhanushkodi

Migrated the review comments from https://github.com/hashicorp/consul-k8s-cli/pull/2 so we can address them here

ndhanushkodi · 2021-09-03T02:59:23Z

cli/cmd/common/flag/doc.go

@@ -0,0 +1,5 @@
+// Package flag is a thin layer over the stdlib flag package that provides


@ reviewers: you don't need to review all of the files in this package unless you want to see how it works since it is from another package

cli/README.md

ndhanushkodi · 2021-09-03T03:02:01Z

cli/cmd/install/install_test.go

@@ -0,0 +1,94 @@
+package install


@kschoche's comment:

These tests look great!
I know that testing CLI bits is challenging, but I'm wondering if we can add testing around skipConfirm and dryRun having expected behaviour?

Does it make sense to have some additional tests around how we're using step, and also somehow have at least a happy-path test of the actual command?

Hey @kschoche, so I tried to give this a shot (skipConfirm and dryRun) and in theory it could have been possible by creating a fake actionConfig that fakes out the kubeclient and helm registry client. I got to the point of almost being able to create a fake actionConfig, but the Helm go SDK has the registry package under an "internal" folder, so I can't import the structs without copying that entire package. Since it would require a bit of jumping through hoops to mock out anything related to Helm, I think it may be worth covering dryRun and skipConfirm and potentially any other behaviour involving the Run() function via acceptance tests?

cli/cmd/install/install.go

cli/README.md

cli/cmd/version/version.go

thisisnotashwin

Heyo!! This is such great work!! The amount of effort that has gone into this is so clear!! Have some comments that should hopefully be straightforward!

cli/cmd/install/presets.go

cli/cmd/install/install_test.go

thisisnotashwin · 2021-09-15T01:11:51Z

cli/cmd/install/install_test.go

+	c.Run([]string{"-auto-approve", "-f=../../config.yaml"})
+}
+
+func TestCheckForPreviousPVCs(t *testing.T) {


This could be migrated to being a table test.

thisisnotashwin · 2021-09-15T01:12:19Z

cli/cmd/install/install_test.go

+	require.NoError(t, err)
+}
+
+func TestCheckForPreviousSecrets(t *testing.T) {


This can be a table test as well.

cli/cmd/install/install_test.go

thisisnotashwin · 2021-09-15T01:16:02Z

cli/cmd/common/base.go

+)
+
+// BaseCommand is embedded in all commands to provide common logic and data.
+type BaseCommand struct {


I love this pattern SO much!!

ndhanushkodi · 2021-09-15T06:03:14Z

Thanks @thisisnotashwin!! I think I've addressed everything but the table tests, so I'm just going to go ahead and re-request your review to see if there's anything else blocking while I fix that tomorrow!

thisisnotashwin

Thanks so much for the quick turnaround on the PR Nitya! This looks great. I think we can migrate to table tests later! Such an excellent PR!!

cli/cmd/install/install.go

t-eckert · 2021-09-15T15:25:10Z

cli/cmd/install/install.go

+		})
+
+		f = c.set.NewSet("Global Options")
+		f.StringVar(&flag.StringVar{


Given that by default people keep the kubeconfig file at ~/.kube/config, I think it would make sense to use that as the default.

In the default case, for any command that uses helm, we'll want to respect Helm's order of evaluation to get the kube context. I think if we were to explicitly set this to ~/.kube/config, then we wouldn't be able to respect the Helm library's "backups" for finding a kubeconfig (i.e an env var, etc.).

Cool, then this is just me not knowing yet :) Thanks!

cli/cmd/install/install.go

t-eckert

Fantastic work, I think this will be a huge improvement for new users coming to Consul on K8s.

consul-k8s install installs the latest Helm chart onto your Kubernetes cluster after performing a few verification steps. common/flag is copied from Waypoint’s internal/pkg/flag. It is a thin layer over the standard Go flag package that provides features such as aliasing and global flags. It was created specifically for mitchellh/cli. common/terminal package is adapted from the Waypoint Plugin SDK and excludes the Status interface and Step interface as described in doc.go. It allows us to have a consistent UI output with style. common/base.go describes a base command that can be embedded into all commands, and pass in common functionality like UI and ctx. This pattern was adapted from Waypoint. Co-authored-by: Saad Jamal <saad@hashicorp.com>

ndhanushkodi · 2021-09-16T04:59:17Z

Thanks so much for the review and fast turnaround on a big PR @t-eckert and @thisisnotashwin!

ndhanushkodi commented Sep 3, 2021

View reviewed changes

ndhanushkodi requested review from thisisnotashwin, a team and t-eckert and removed request for a team September 10, 2021 05:38

ndhanushkodi force-pushed the cli-install branch 2 times, most recently from 953b3a7 to e77da79 Compare September 10, 2021 06:02

t-eckert reviewed Sep 14, 2021

View reviewed changes

cli/README.md Outdated Show resolved Hide resolved

cli/cmd/version/version.go Show resolved Hide resolved

thisisnotashwin reviewed Sep 15, 2021

View reviewed changes

ndhanushkodi mentioned this pull request Sep 15, 2021

cli: uninstall command #725

Merged

2 tasks

ndhanushkodi requested a review from thisisnotashwin September 15, 2021 06:04

thisisnotashwin approved these changes Sep 15, 2021

View reviewed changes

cli/cmd/install/install.go Show resolved Hide resolved

t-eckert reviewed Sep 15, 2021

View reviewed changes

cli/cmd/install/install.go Outdated Show resolved Hide resolved

t-eckert reviewed Sep 15, 2021

View reviewed changes

cli/cmd/install/install.go Outdated Show resolved Hide resolved

t-eckert approved these changes Sep 15, 2021

View reviewed changes

ndhanushkodi force-pushed the cli-install branch 2 times, most recently from 4857b15 to 749c942 Compare September 16, 2021 01:43

ndhanushkodi and others added 2 commits September 15, 2021 19:30

cli: add LibraryStyle for helm output and add ✓ for success output

5f3b109

ndhanushkodi force-pushed the cli-install branch from 8b22214 to 5f3b109 Compare September 16, 2021 02:30

ndhanushkodi added 2 commits September 15, 2021 21:40

cli: add -wait and -timeout flags to install and update readme

7508187

cli: add changelog

15ecc6a

ndhanushkodi changed the title ~~cli: install command (and move it to main repository)~~ cli: support install command Sep 16, 2021

ndhanushkodi merged commit 9496831 into main Sep 16, 2021

ndhanushkodi deleted the cli-install branch September 16, 2021 05:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: support install command #713

cli: support install command #713

ndhanushkodi commented Sep 3, 2021 •

edited

Loading

ndhanushkodi left a comment

ndhanushkodi Sep 3, 2021

ndhanushkodi Sep 3, 2021

ndhanushkodi Sep 9, 2021

thisisnotashwin left a comment

thisisnotashwin Sep 15, 2021

thisisnotashwin Sep 15, 2021

thisisnotashwin Sep 15, 2021

ndhanushkodi commented Sep 15, 2021 •

edited

Loading

thisisnotashwin left a comment

t-eckert Sep 15, 2021

ndhanushkodi Sep 16, 2021

t-eckert Sep 16, 2021

t-eckert left a comment

ndhanushkodi commented Sep 16, 2021

		@@ -0,0 +1,5 @@
		// Package flag is a thin layer over the stdlib flag package that provides

cli: support install command #713

cli: support install command #713

Conversation

ndhanushkodi commented Sep 3, 2021 • edited Loading

ndhanushkodi 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

thisisnotashwin 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

ndhanushkodi commented Sep 15, 2021 • edited Loading

thisisnotashwin 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

t-eckert left a comment

Choose a reason for hiding this comment

ndhanushkodi commented Sep 16, 2021

ndhanushkodi commented Sep 3, 2021 •

edited

Loading

ndhanushkodi commented Sep 15, 2021 •

edited

Loading