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

Use Helm-style parameters for kudoctl kudo install command #283

Merged
merged 3 commits into from
Jun 5, 2019

Conversation

zen-dog
Copy link
Contributor

@zen-dog zen-dog commented Jun 3, 2019

So instead of kubectl kudo install kafka -p foo,bar it would be: kubectl kudo install kafka -p foo=bar

Additionally, parameter validation is now done before the actual command execution to avoid potential failures down the road.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/component generator

/component kudoctl

/component operator
/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind feature
/kind enhancement
/kind infrastructure
/kind kep

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #225

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`kudoctl kudo install` command now uses Helm-style parameters e.g. `kubectl kudo install kafka -p foo=bar`

Co-authored-by: Alena Varkockova [email protected]

…ead of:

`kubectl kudo install kafka -p foo,bar` it would be: `kubectl kudo install kafka -p foo=bar`

Additionally, parameter validation is now done before the actual command execution to avoid potential failures down the road.

Issue: #225
@zen-dog zen-dog requested review from alenkacz and gerred June 3, 2019 17:15
@zen-dog zen-dog changed the title Use helm style parameters for kudoctl kudo install command Use Helm-style parameters for kudoctl kudo install command Jun 3, 2019
@gerred
Copy link
Member

gerred commented Jun 3, 2019

This may be more of a Cobra and "how does my particular shell work" question, but how does this work if a value is wrapped in quotes? Does it get parsed into a string without the quotes (unless they're escaped)?

If we need to handle that detail, make sure the validate func handles it.

pkg/kudoctl/cmd/install_test.go Outdated Show resolved Hide resolved
Copy link
Member

@fabianbaier fabianbaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but let's also aim to update the documentation https://kudo.dev/docs/cli/#install-a-package-with-instancename--parameters with this PR?

kubectl kudo install kafka --instance=my-kafka-name --parameter KAFKA_ZOOKEEPER_URI=zk-zk-0.zk-hs:2181,zk-zk-1.zk-hs:2181,zk-zk-2.zk-hs:2181 --parameter KAFKA_ZOOKEEPER_PATH=/small -p BROKERS_COUNTER=3

pkg/kudoctl/cmd/install/install.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/install/install.go Show resolved Hide resolved
@gerred
Copy link
Member

gerred commented Jun 3, 2019

@alenkacz @zen-dog I think my comments would establish the CLI's role in parameter parsing (splitting on =) and validation of a Parameter into separate packages that can be worked with, and tested individually. Parameter could then be expanded to be the same struct as the Framework SDK and offer a lot of code re-use and richer functionality.

@fabianbaier
Copy link
Member

This may be more of a Cobra and "how does my particular shell work" question, but how does this work if a value is wrapped in quotes? Does it get parsed into a string without the quotes (unless they're escaped)?

If we need to handle that detail, make sure the validate func handles it.

agreed, right now

-pKAFKA_ZOOKEEPER_URI=zk-zk-0.zk-hs:2181,zk-zk-1.zk-hs:2181,"zk-zk-2.zk-hs:2181"

will be parsed without the quotes.

When then (kubectl describe instance my-kafka) e.g. it would be turned into

KAFKA_ZOOKEEPER_URI: zk-zk-0.zk-hs:2181,zk-zk-1.zk-hs:2181,zk-zk-2.zk-hs:2181

Just when escaping via

--parameter KAFKA_ZOOKEEPER_URI=zk-zk-0.zk-hs:2181,zk-zk-1.zk-hs:2181,\"zk-zk-2.zk-hs:2181\"

into

KAFKA_ZOOKEEPER_URI: zk-zk-0.zk-hs:2181,zk-zk-1.zk-hs:2181,"zk-zk-2.zk-hs:2181"

you see the quotes. I like the idea of having the validate func handle this.

pkg/kudoctl/cmd/install_test.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/install/install.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/install/install.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/install/install.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/install/install.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
@gerred gerred added component/cli kubectl kudo a.k.a. kudoctl kind/enhancement labels Jun 4, 2019
- used table-driven tests for `TestTableNewInstallCmd_WithParameters`
- `validateInstallParameters` now returns all invalid parameters and not just the first one
@gerred gerred self-requested a review June 4, 2019 15:45
@gerred gerred requested a review from gkleiman June 4, 2019 16:07
@gerred gerred merged commit 7bd9758 into master Jun 5, 2019
@gerred gerred deleted the ad/helm-style-install-parameters branch June 5, 2019 13:40
@fabianbaier fabianbaier mentioned this pull request Jun 6, 2019
@porridge porridge added release/highlight This PR is a highlight for the next release and removed release-highlight labels Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/cli kubectl kudo a.k.a. kudoctl kind/enhancement release/highlight This PR is a highlight for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Install CLI to use Helm-style parameters
6 participants