Support passing boolean flags to kubectl plugins #58768

carolynvs · 2018-01-24T20:47:39Z

What this PR does / why we need it:
This allows kubectl plugins to define boolean flags. Boolean flags are set to true when the flag is present and no value is specified. --flag is equivalent to --flag=true and is useful for commands such as kubectl plugin svcat --version.

To preserve backwards compatibility with existing plugins, all flags by default are string flags, but now a new field has been introduced to the flag definition, type, which allows specifying the type of pflag to use. Currently only string and bool are supported, but this allows for other types to be added in the future.

Bool flags support a default value, and when defValue is set, kubectl validates that it can be parsed as a boolean when validating the plugin. Acceptable values can be found at https://golang.org/pkg/strconv/#ParseBool.

Example

name: "svcat"
shortDesc: "Service Catalog CLI"
command: "./svcat"
flags:
  - name: version
    desc: Display the version of the plugin
    type: bool

Which issue(s) this PR fixes
Fixes #53640

Special notes for your reviewer:
None

Release note:

Support passing boolean flags to kubectl plugins

/sig cli

ericchiang · 2018-01-24T22:26:59Z

/ok-to-test

cc @fabianofranz are you a good person to review? Maybe someone else from sig/cli?

ericchiang · 2018-01-29T18:34:15Z

cc @kubernetes/sig-cli-pr-reviews

dixudx · 2018-01-30T01:38:48Z

pkg/kubectl/cmd/plugin.go

-		cmd.Flags().StringP(flag.Name, flag.Shorthand, flag.DefValue, flag.Desc)
+		switch flag.Type {
+		// When a flag type is not defined, treat it as a string flag (original behavior before type was introduced)
+		case "", plugins.StringFlagType:


IMO, better make it as default.

dixudx · 2018-01-30T01:54:45Z

pkg/kubectl/plugins/plugins.go

+			return ErrInvalidFlagType
+		}
+	}
+	if f.Type == BoolFlagType && f.DefValue != "" {


What if DefValue is empty for boolean type?

You're right, I'll update the patch so that it defaults to false when not specified for boolean flags.

dixudx · 2018-01-30T01:55:28Z

pkg/kubectl/plugins/plugins.go

+)
+
+// flagTypes defines the set of supported flag types.
+var flagTypes = map[FlagType]struct{}{BoolFlagType: {}, StringFlagType: {}}


I prefer to move this into function Validate. No need to expose here.

dixudx · 2018-01-30T01:56:38Z

pkg/kubectl/plugins/plugins.go

@@ -105,6 +127,16 @@ func (f Flag) Validate() error {
 	if strings.Contains(f.Name, " ") {
 		return ErrInvalidFlagName
 	}
+	if f.Type != "" {


We should make this backwards compatible. If the flag type is missing or empty string, we should take it as string flag by default.

dixudx · 2018-01-30T02:15:02Z

pkg/kubectl/plugins/plugins.go

-	Desc      string `json:"desc"`
-	DefValue  string `json:"defValue,omitempty"`
+	Name      string   `json:"name"`
+	Type      FlagType `json:"type"`


To make it backwards compatible, this new-added field should be optional.

Should be json:"type,omitempty" here.

carolynvs · 2018-02-05T19:49:19Z

Thanks for the feedback, @dixudx! I've updated the PR to incorporate your suggestions.

dixudx

Only a small nit. LGTM mostly.

dixudx · 2018-02-06T01:54:18Z

pkg/kubectl/plugins/plugins.go

@@ -33,6 +34,10 @@ var (
 	ErrInvalidFlagName = fmt.Errorf("flag name can't contain spaces")
 	// ErrInvalidFlagShorthand indicates flag shorthand is invalid.
 	ErrInvalidFlagShorthand = fmt.Errorf("flag shorthand must be only one letter")
+	// ErrInvalidFlagType indicates flag type is invalid.
+	ErrInvalidFlagType = fmt.Errorf("invalid flag type, must be one of the following: string, bool")


Adding default to bool?

How about invalid flag type, must be one of the following: string, bool(default)?

dixudx · 2018-02-06T02:09:06Z

@carolynvs Please also squash your commits. Then lgtm. Thanks.

Boolean flags are set to true when the flag is present and no value is specified. --flag is equivalent to --flag=true and is useful for commands such as a --version flag. To preserve backwards compatibiity with existing plugins, all flags by default are string flags, but now a new field has been introduced to the flag definition, type, which allows specifying the type of pflag to use. Currently only string and bool are supported, but this allows for other types to be added in the future. Bool flags support a default value, and when defValue is set, kubectl validates that it can be parsed as a boolean when validating the plugin. Acceptable values can be found at https://golang.org/pkg/strconv/#ParseBool.

carolynvs · 2018-02-06T15:38:31Z

@dixudx I've updated the error message to indicate that strings are the default type and squashed my commits.

dixudx · 2018-02-06T15:41:03Z

/lgtm
@carolynvs Good job! Thanks for your work.

carolynvs · 2018-02-06T16:10:54Z

/assign @deads2k

soltysh

/lgtm

ericchiang · 2018-02-12T23:16:06Z

test failure looks unrelated

/test pull-kubernetes-bazel-test

dixudx · 2018-02-13T06:01:06Z

Ping @kubernetes/sig-cli-maintainers for approval. Thanks.

k8s-github-robot · 2018-02-28T08:46:54Z

/test all

Tests are more than 96 hours old. Re-running tests.

carolynvs · 2018-03-05T14:04:31Z

/retest

thockin · 2018-03-05T14:04:40Z

This change is

carolynvs · 2018-03-05T14:57:22Z

None of the failing tests are related to my PR. I'm not quite sure what I should do next? 🤷‍♀️

adohe-zz · 2018-03-10T03:22:14Z

/test pull-kubernetes-e2e-gce

adohe-zz · 2018-03-10T03:22:50Z

None of the failing tests are related to my PR. I'm not quite sure what I should do next?

I just re-start the failed test case, lets see what's the result.

adohe-zz · 2018-03-10T03:26:22Z

This overall looks good to me, I would like to get this merged asap. Then I will open a new issue to put things which we need to do before we can move kubectl plugin to beta.

adohe-zz · 2018-03-10T03:26:34Z

/assign @adohe

adohe-zz · 2018-03-10T03:28:45Z

Seems milestone freeze, could be get merged after the 1.10 release.

liggitt · 2018-03-10T03:43:28Z

This overall looks good to me, I would like to get this merged asap. Then I will open a new issue to put things which we need to do before we can move kubectl plugin to beta.

The current plugin design led to the necessity for this pull. Before going further down the road of requiring plugins to describe all their flags to kubectl, it’s worth discussing whether that is the correct approach at all. That approach stunts plugins by requiring all their flags to be parseable by kubectl. I’d be strongly in favor of just passing through all envvars, flags and input/output streams as is and letting plugins run as if you had just invoked them directly.

k8s-ci-robot · 2018-03-10T04:13:31Z

@carolynvs: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce	`4aa04d8`	link	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

carolynvs · 2018-03-12T13:55:29Z

I’d be strongly in favor of just passing through all envvars, flags and input/output streams as is and letting plugins run as if you had just invoked them directly.

@liggitt I could not agree more that having kubectl parse the Service Catalog's plugin flags makes things much more difficult for us, and it's not clear what is being gained. Our plugin is a binary that uses cobra, so for us it's a pointless exercise to generate the plugin yaml, use viper to rebind the envvars back to the original flags, etc. If there is a conversation about this going on elsewhere, I'd love to provide feedback! 😀

deads2k · 2018-03-12T14:13:23Z

@liggitt I could not agree more that having kubectl parse the Service Catalog's plugin flags makes things much more difficult for us, and it's not clear what is being gained. Our plugin is a binary that uses cobra, so for us it's a pointless exercise to generate the plugin yaml, use viper to rebind the envvars back to the original flags, etc. If there is a conversation about this going on elsewhere, I'd love to provide feedback!

We're mostly busy closing down 1.10, but I think that we should reconsider how plugins for kubectl work. Straight pass-through with execve avoids unnecessary meta, allows easier testing, and makes the plugged in binaries make sense both with and without kubectl.

@soltysh

soltysh · 2018-03-12T14:27:36Z

@adohe @carolynvs let's discuss the idea @deads2k voiced out during our next sig-cli call. That's basically the direction I'd like us take before we promote this to beta.
/lgtm cancel

k8s-ci-robot · 2018-03-12T14:28:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: carolynvs, dixudx
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: adohe

Assign the PR to them by writing /assign @adohe in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

pkg/kubectl/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt · 2018-03-12T14:43:50Z

c.f. sig-cli meeting info (https://github.com/kubernetes/community/tree/master/sig-cli) and agenda (https://docs.google.com/document/d/1r0YElcXt6G5mOWxwZiXgGu_X6he3F--wKwg-9UBc29I/edit#)

carolynvs · 2018-04-27T17:13:17Z

Now that 1.10 is out, is this something that can be merged to help make the current plugin experience better?

fejta-bot · 2018-07-26T17:19:53Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

carolynvs · 2018-07-26T17:27:39Z

/remove-lifecycle stale

carolynvs · 2018-07-26T17:29:20Z

Can anyone from SIG CLI comment on if the plan is to not make any further fixes to the plugin infrastructure because something else is coming down the pipeline? I'm unsure if I should just close this PR, or keep trying to get this merged.

soltysh · 2018-08-24T14:37:10Z

@carolynvs have a look at kubernetes/community#2437 and #66876 which should address all your problems. This new mechanism should unblock all the problems you were struggling with before. Ping me if you have any questions/problems.
/close

carolynvs · 2018-08-24T15:59:16Z

@soltysh This is great, thanks! Looking forward to the next release of k8s. I'm working with SIG Service Catalog to get svcat updated to work with the new model. 👍

k8s-ci-robot requested review from adohe-zz and ericchiang January 24, 2018 20:47

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 24, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 24, 2018

dixudx reviewed Jan 30, 2018

View reviewed changes

dixudx reviewed Feb 6, 2018

View reviewed changes

carolynvs force-pushed the kubectl-plugin-bool-flag branch from be8883b to 4aa04d8 Compare February 6, 2018 15:36

k8s-ci-robot assigned dixudx Feb 6, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2018

k8s-ci-robot assigned deads2k and soltysh Feb 6, 2018

soltysh approved these changes Feb 7, 2018

View reviewed changes

carolynvs mentioned this pull request Mar 8, 2018

[plugin] Value must be assigned for boolean flags kubernetes/kubectl#345

Closed

k8s-ci-robot assigned adohe-zz Mar 10, 2018

adohe-zz approved these changes Mar 10, 2018

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 12, 2018

christopherhein mentioned this pull request Apr 18, 2018

Support for "worker" and "master" flags to randomly pick an instance christopherhein/kubectl-ssh#1

Open

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 26, 2018

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 26, 2018

k8s-ci-robot closed this Aug 24, 2018

carolynvs deleted the kubectl-plugin-bool-flag branch August 24, 2018 15:59

Support passing boolean flags to kubectl plugins #58768

Support passing boolean flags to kubectl plugins #58768

Conversation

carolynvs commented Jan 24, 2018

ericchiang commented Jan 24, 2018

ericchiang commented Jan 29, 2018

dixudx Jan 30, 2018

Choose a reason for hiding this comment

dixudx Jan 30, 2018

Choose a reason for hiding this comment

carolynvs Feb 5, 2018

Choose a reason for hiding this comment

dixudx Jan 30, 2018

Choose a reason for hiding this comment

dixudx Jan 30, 2018

Choose a reason for hiding this comment

dixudx Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

carolynvs commented Feb 5, 2018

dixudx left a comment

Choose a reason for hiding this comment

dixudx Feb 6, 2018

Choose a reason for hiding this comment

dixudx commented Feb 6, 2018

carolynvs commented Feb 6, 2018 • edited Loading

dixudx commented Feb 6, 2018

carolynvs commented Feb 6, 2018

soltysh left a comment

Choose a reason for hiding this comment

ericchiang commented Feb 12, 2018

dixudx commented Feb 13, 2018

k8s-github-robot commented Feb 28, 2018

carolynvs commented Mar 5, 2018

thockin commented Mar 5, 2018

carolynvs commented Mar 5, 2018

adohe-zz commented Mar 10, 2018

adohe-zz commented Mar 10, 2018

adohe-zz commented Mar 10, 2018

adohe-zz commented Mar 10, 2018

adohe-zz commented Mar 10, 2018

liggitt commented Mar 10, 2018 • edited Loading

k8s-ci-robot commented Mar 10, 2018

carolynvs commented Mar 12, 2018

deads2k commented Mar 12, 2018

soltysh commented Mar 12, 2018

k8s-ci-robot commented Mar 12, 2018

liggitt commented Mar 12, 2018

carolynvs commented Apr 27, 2018

fejta-bot commented Jul 26, 2018

carolynvs commented Jul 26, 2018

carolynvs commented Jul 26, 2018

soltysh commented Aug 24, 2018

carolynvs commented Aug 24, 2018

dixudx Jan 30, 2018 •

edited

Loading

carolynvs commented Feb 6, 2018 •

edited

Loading

liggitt commented Mar 10, 2018 •

edited

Loading