Add flags to svcat register broker command #2208

jberkhahn · 2018-07-17T21:04:27Z

Also contains refactoring of register to change mandatory flags into positional args.

Also adds an options struct to pkg/svcat for containing the bajillion different flag options. This also makes the interface around Register() much more stable.

I'm centering validation in the cmd layer's Validate() method (from cobra). So pkg/svcat's Register() method will do strange things if you ask it to.

MHBauer · 2018-07-17T21:24:21Z

if you put WIP into the title of the PR it automatically applies the label.

MHBauer · 2018-07-17T21:25:53Z

I dunno how to read regexes, but here https://github.com/kubernetes/test-infra/blob/master/prow/plugins/wip/wip-label.go#L41

carolynvs

This is looking good! 👍

FYI, I didn't review the tests yet, I'll look at those once the desired behavior is clarified.

carolynvs · 2018-07-25T21:27:59Z

cmd/svcat/broker/register_cmd.go

+	PlanRestrictions  []string
+	SkipTLS           bool
+	RelistBehavior    string
+	RelistDuration    time.Duration


I'm always confused by what a create should look like with a manual relistduration. Should we pass a null duration or does the api server just ignore whatever is sent for the duration?

This sends a time.Duration of 0, which the apiserver drops on the floor. It never gets set in the spec at all.

carolynvs · 2018-07-25T21:29:09Z

cmd/svcat/broker/register_cmd.go

 	}
 	cmd := &cobra.Command{
-		Use:   "register NAME --url URL",
+		Use:   "register NAME URL",


lol I see what you did there. 😂 My vote is still for keeping --url.

carolynvs · 2018-07-25T21:30:40Z

cmd/svcat/broker/register_cmd.go

-	cmd.Flags().StringVar(&registerCmd.URL, "url", "",
-		"The broker URL (Required)")
-	cmd.MarkFlagRequired("url")
+	cmd.Flags().StringVar(&registerCmd.BasicSecret, "basic-secret", "",


Do you know how the secrets work when we are making a ClusterServiceBroker vs a ServiceBroker. For a namespaced broker, I assume that the secret is in the same namespace. For a cluster-scoped broker though, do they have to provide a namespace? Or is it just embedded in the format, e.g. "namespace/secret-name"?

Currently it tags the secret as being in whatever namespace the command was executed in. This might be a problem when we want to change the command to create namespaced broker.

carolynvs · 2018-07-25T21:32:40Z

cmd/svcat/broker/register_cmd.go

+	cmd.Flags().StringVar(&registerCmd.CAFile, "ca", "",
+		"A file containing the CA certificate to connect to the broker")
+	cmd.Flags().StringSliceVar(&registerCmd.ClassRestrictions, "class-restrictions", []string{},
+		"A whitelist of Classes to allow from the broker")


Let's not use blacklist/whitelist and stick with the terms that are used in the documentation:

https://svc-cat.io/docs/catalog-restrictions/

I recommend "A list of restrictions to apply to the classes allowed from the broker" or something like that.

carolynvs · 2018-07-25T21:33:26Z

cmd/svcat/broker/register_cmd.go

+	cmd.Flags().StringSliceVar(&registerCmd.PlanRestrictions, "plan-restrictions", []string{},
+		"A whitelist of Plans to allow from the broker")
+	cmd.Flags().StringVar(&registerCmd.RelistBehavior, "relist-behavior", "",
+		"Behavior for relisting the broker's catalogue. Manual/Duration")


I think we use the US spelling "catalog" everywhere else, so let's rename "catalogue" to "catalog" for consistency.

The pattern that we have used elsewhere for allowed values is:

lower cased

use a comma separated list of allowed values, examples below

https://github.com/kubernetes-incubator/service-catalog/blob/f31d3e56fde00dc6f9012675d10d7ccfafba1762/cmd/svcat/command/formatted.go#L49

https://github.com/kubernetes-incubator/service-catalog/blob/0a421721ee69ab5a8a89ecf279a326a163adcb4d/cmd/svcat/command/scoped.go#L59

carolynvs · 2018-07-25T21:42:16Z

cmd/svcat/broker/register_cmd.go

+	cmd.Flags().StringVar(&registerCmd.RelistBehavior, "relist-behavior", "",
+		"Behavior for relisting the broker's catalogue. Manual/Duration")
+	cmd.Flags().DurationVar(&registerCmd.RelistDuration, "relist-duration", 0*time.Second,
+		"Interval to refetch broker catalog when relist-behavior is set to duration, specified in human readable format: 30s, 1m, 1h")


This implies that we only use relist-duration when the behavior is manual, is that right?

Uh, what? Its the interval to (do thing) when relist behavior is set to duration. That means we only use it when the behavior is duration.

right?

carolynvs · 2018-07-25T21:44:30Z

cmd/svcat/broker/register_cmd.go

+	if c.RelistBehavior != "" {
+		c.RelistBehavior = strings.ToLower(c.RelistBehavior)
+		if c.RelistBehavior != "duration" && c.RelistBehavior != "manual" {
+			return fmt.Errorf("relist-behavior must be duration/manual")


Let's use "invalid --relist-duration value, allowed values are: duration, manual" to match our other error messages.

carolynvs · 2018-07-25T21:46:34Z

cmd/svcat/broker/register_cmd.go

+			return fmt.Errorf("relist-behavior must be duration/manual")
+		}
+	}
+	if c.RelistDuration != 0*time.Second && c.RelistBehavior != "duration" {


Is this something that the api server would accept? I am wondering if we are applying validations on the cli to values that the server would allow. Can you check please?

api server drops the relist duration on the ground. I guess we can remove this.

carolynvs · 2018-07-25T21:48:16Z

cmd/svcat/broker/register_cmd.go

 	return nil
 }

-// Run runs the command
+// Run calls out to the pkg lib to create the broker and displays the output


nit: let's not put the call pattern into the comment. How about // Run creates the broker and then displays the broker details

carolynvs · 2018-07-25T21:50:34Z

cmd/svcat/broker/register_cmd.go

+		BearerSecret:      c.BearerSecret,
+		CAFile:            c.CAFile,
+		ClassRestrictions: c.ClassRestrictions,
+		Namespace:         c.Context.App.CurrentNamespace,


Do not directly use the current namespace, because that doesn't properly take into account all of the necessary logic. The Namespaced struct handles defaulting this appropriately, and the final namespace can be found in c.Namespace.

how does that interact with throwing the global kubectl namespace flag when running in plugin mode?

jberkhahn · 2018-07-27T19:43:30Z

redid the namespace stuff using namespaced

carolynvs · 2018-08-08T14:40:58Z

cmd/svcat/broker/register_cmd.go

 	}
 	cmd := &cobra.Command{
-		Use:   "register NAME --url URL",
+		Use:   "register NAME URL",


Please change this back to --url.

General consensus at the face to face and in the meeting before that was to move to positional arguments and get rid of mandatory flags. Do you want to bring this up again next week in the meeting to discuss?

carolynvs · 2018-08-08T14:49:09Z

cmd/svcat/broker/register_cmd.go

+			return fmt.Errorf("invalid --relist-duration value, allowed values are: duration, manual")
+		}
+	}
+	/**


Let's remove this dead code / comment.

carolynvs · 2018-08-08T14:50:16Z

pkg/svcat/service-catalog/assets/ca

@@ -0,0 +1 @@
+foo


Best CA cert ever ⭐️

jeremyrickard · 2018-08-08T15:23:03Z

cmd/svcat/broker/register_cmd.go

 	}
 	cmd := &cobra.Command{
-		Use:   "register NAME --url URL",


why did we change this? I'm not sure I really like the positional arguments like this. The cf CLI does this, but I think kubernetes gets more into -- args vs positional things and I think we should stick with that.

we discussed this at the face-to-face. We decided to switch to positional args as mandatory flags goes against unix command line convention.

While this isn't the hill I want to die on 😝, I honestly do not remember that being officially decided at the F2F as you are recalling.

@jberkhahn I apologize for this being so weird. I trust that we did decide that and I just forgot the details. Let's go with what you have here and I now crown you master and commander of the CLI so that we don't have to rely on my poor memory anymore!

hahhahah I was thinking of a 👑 , jeremy immediately went to that ⛵️ movie and I think that's a beefeater? best title ever! 🙌

I must have been on one of those bathroom breaks - - I too don't recall the position vs named arg discussion/decision.

No? We also discussed this in the weekly meeting the week before the f2f and had the same consensus. We can bring it up again at the meeting next week if you prefer.

jeremyrickard · 2018-08-09T16:42:56Z

/hold

jeremyrickard · 2018-08-09T16:44:08Z

@jberkhahn @jboyd01 @carolynvs I put this on hold for now, I feel like we should chat about it Monday since there seems to be some confusion about args. Let's just revisit the topic and then we can come back to this!

removed the changes in question

carolynvs

/lgtm

- add options object to pkg/svcat - change svcat register --url flag to positional argument

carolynvs · 2018-08-14T18:54:42Z

/lgtm

jeremyrickard · 2018-08-14T19:02:12Z

/approve

k8s-ci-robot · 2018-08-14T19:02:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeremyrickard

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:

~~cmd/svcat/OWNERS~~ [jeremyrickard]
~~pkg/svcat/OWNERS~~ [jeremyrickard]

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

jboyd01 · 2018-08-14T19:07:41Z

cmd/svcat/broker/register_cmd.go

@@ -48,27 +68,73 @@ func NewRegisterCmd(cxt *command.Context) *cobra.Command {
 	cmd.Flags().StringVar(&registerCmd.URL, "url", "",
 		"The broker URL (Required)")
 	cmd.MarkFlagRequired("url")
+	cmd.Flags().StringVar(&registerCmd.BasicSecret, "basic-secret", "",
+		"A secret containing basic auth (username/password) information to connect to the broker")
+	cmd.Flags().StringVar(&registerCmd.BearerSecret, "bearer-secret", "",


Should basic-secret and bearer-secret not be basic-secret-name and bearer-secret-name?

We don't follow that convention for flags that indicate secrets for other commands - both provision and bind just use -secret, not -secret-name

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 17, 2018

k8s-ci-robot requested review from luksa and n3wscott July 17, 2018 21:04

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 17, 2018

jberkhahn added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2018

jberkhahn force-pushed the svcat_register_flags branch 2 times, most recently from d551332 to d02d379 Compare July 17, 2018 23:24

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2018

jberkhahn force-pushed the svcat_register_flags branch from 89d9d62 to cd16a6f Compare July 18, 2018 22:45

jberkhahn added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2018

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 24, 2018

jberkhahn force-pushed the svcat_register_flags branch from a6d2fb5 to 35314a0 Compare July 24, 2018 21:41

jberkhahn removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2018

jberkhahn requested a review from carolynvs July 24, 2018 21:43

jberkhahn force-pushed the svcat_register_flags branch 2 times, most recently from f1b9e85 to bde82bc Compare July 24, 2018 22:15

carolynvs suggested changes Jul 25, 2018

View reviewed changes

jberkhahn force-pushed the svcat_register_flags branch 5 times, most recently from b782122 to e91bf56 Compare July 26, 2018 20:50

jberkhahn assigned jeremyrickard and unassigned jeremyrickard Jul 27, 2018

jberkhahn requested review from artmello and jeremyrickard July 27, 2018 00:20

jberkhahn force-pushed the svcat_register_flags branch from e91bf56 to 8f9245a Compare July 30, 2018 17:49

carolynvs suggested changes Aug 8, 2018

View reviewed changes

jeremyrickard previously requested changes Aug 8, 2018

View reviewed changes

jberkhahn force-pushed the svcat_register_flags branch from 8f9245a to b3280e5 Compare August 8, 2018 18:34

carolynvs approved these changes Aug 8, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2018

jberkhahn force-pushed the svcat_register_flags branch from b3280e5 to 9a46552 Compare August 13, 2018 20:55

jberkhahn removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2018

carolynvs approved these changes Aug 13, 2018

View reviewed changes

k8s-ci-robot assigned carolynvs Aug 13, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2018

jberkhahn force-pushed the svcat_register_flags branch from 9a46552 to eeccfda Compare August 13, 2018 21:06

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2018

Add flags to svcat register command

358d448

- add options object to pkg/svcat - change svcat register --url flag to positional argument

jberkhahn force-pushed the svcat_register_flags branch from eeccfda to 358d448 Compare August 13, 2018 21:17

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2018

jboyd01 reviewed Aug 14, 2018

View reviewed changes

k8s-ci-robot merged commit eb684a9 into kubernetes-retired:master Aug 14, 2018

		@@ -0,0 +1 @@
		foo

Add flags to svcat register broker command #2208

Add flags to svcat register broker command #2208

Conversation

jberkhahn commented Jul 17, 2018 • edited Loading

MHBauer commented Jul 17, 2018

MHBauer commented Jul 17, 2018

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

jberkhahn commented Jul 27, 2018

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

jeremyrickard commented Aug 9, 2018

jeremyrickard commented Aug 9, 2018

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs commented Aug 14, 2018

jeremyrickard commented Aug 14, 2018

k8s-ci-robot commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Jul 17, 2018 •

edited

Loading