Move "External" around in some resource names/properties by duglin · Pull Request #1354 · kubernetes-retired/service-catalog

duglin · 2017-10-10T00:43:03Z

Per https://github.com/kubernetes-incubator/service-catalog/pull/1350/files#r143453045 I see no value to the end-user to have the word "External" there. They shouldn't know, or care, if the service is external or not, and in fact it may not be.

vaikas · 2017-10-10T01:01:19Z

I'm fine with this change, I think External prefix is wordy. The original reasoning for this was to make sure there's no confusion between k8s names and SC names. @pmorie recall the details?

vaikas · 2017-10-10T01:09:38Z

 spec:
-  externalClusterServiceClassName: test-serviceclass
-  externalClusterServicePlanName: example-plan-1
+  clusterServiceClassName: test-serviceclass


Wonder if there's way to find a way to get rid of the cluster* prefix. At first I thought, great because it's in the cluster service instance, but it's not. if there was a way to get the context 'for free' as being cluster or namespaced, would be nice to not have to specify it here.

I suspect we may allow for the same name to be used at both scopes, which mean we'll need a way for the user to tell us which one they mean.

Yeah, I think we need to keep cluster prefix for the forward compatibility with namespace-scoped counterparts in future.

@nilebox is correct; let's not do this precise rename

nilebox

Just one nit

nilebox · 2017-10-10T04:23:29Z


-const externalClusterServiceClassName = "test-serviceclass"
-const externalClusterServicePlanName = "test-plan"
+const ClusterServiceClassName = "test-serviceclass"


should be kept private probably (camel-case)

vaikas · 2017-10-10T21:11:21Z

How is this going to look with
#1285
in play?

duglin · 2017-10-10T21:17:31Z

what if we used some other prefix because "External"? Something that might actually mean something to the end-user? Like "ClusterServiceClassCliName"?

Either way, it seems to me that the prefix should go next to "Name" not before the entire string.

Thoughts?

nilebox · 2017-10-10T22:04:37Z

it seems to me that the prefix should go next to "Name" not before the entire string.

I totally agree with that. My point though is that it should be the same as the field name in the resource we are referring to.

Is it called ExternalName in ClusterServiceClass? Then the reference field should be called clusterServiceClassExternalName, period.

Referring to OSB or CLI or whatever you want user to be aware of is confusing to me.

nilebox · 2017-10-10T22:08:19Z

@duglin if you are unhappy with External prefix, you should rename it in the ClusterServiceClass first to be consistent.

duglin · 2017-10-10T22:10:59Z

@nilebox agreed on both points. So what do people think about calling the property CliName instead of ExternalName?

nilebox · 2017-10-10T22:23:51Z

@duglin where do you get reference to "Cli" from? CLI is a client-side tool which has nothing to do with svc-cat API server. Our project, Smith, for example, talks directly to the Service Catalog server, and having to deal with CliName would be very confusing.

If anything, I can live with osbName or brokerName as a less ambiguous term than externalName.

duglin · 2017-10-10T22:31:58Z

I picked "cli" because I was thinking that it was what we would have users (cli-folks) to use. But, I'm ok with just about anything that's might make more sense to the end user than "External", which is meaningless to me and feels more like we're exposing the inner-workings to people, which they shouldn't care about. "osb" might be ok but I'd prefer for people not to even know that OSB is at play here. I mean technically "HumanReadable" :-) is good but of course it also sucks because its too long.

Today: ClusterServiceClassSpec.ExternalName
Maybe:
 ClusterServiceClassSpec.NiceName
 ClusterServiceClassSpec.ShortName
 ClusterServiceClassSpec.HumanName
 ClusterServiceClassSpec.DisplayName

Short or Display perhaps?

nilebox · 2017-10-11T00:06:22Z

@duglin displayName is kinda fine but also as vague as externalName, so I don't see any improvement there... The rest suggest that OSB name is always shorter and nicer, which is not necessarily true, as it is an arbitrary string that can be UUID or something else.

Until we come up with consensus on a better name, can you make change with ClusterServiceClassSpecExternalName for now?

vaikas · 2017-10-11T16:44:36Z

I'm not a fan of making a long name longer and reordering it 'until we come up with consensus' approach given the churn it will produce.

duglin · 2017-10-11T17:25:28Z

The OSB spec defines service.name as: A CLI-friendly name of the service. MUST only contain lowercase characters, numbers and hyphens (no spaces). Same text for plan.name.

So I still kind of like: ClusterServiceClassSpec.CliName

If/when we get around to having a nice UX kubectl plugin then this is the name they will specify - so consistency between the cli and yaml would be good.

vaikas · 2017-10-13T21:15:22Z

I'm fine with CLI in the name. I think it's consistent with OSB spec.

nilebox · 2017-10-13T21:45:26Z

A CLI-friendly name of the service

Dunno, to me it sounds like CFism. OSB spec doesn't (and shouldn't) have any requirements for marketplace to provide a CLI interface at all, so I am not even sure which CLI does it refer to. I get the "human-readable name" goal, just don't think it has anything to do with CLI. What if marketplace has a user-friendly UI (which needs human-readable names too), but doesn't provide a CLI interface? Does it make marketplace non-OSB-compliant? I don't think so.

nilebox · 2017-10-13T21:58:16Z

Key maybe (ClusterServiceClassKey)? For me this word is intuitively interpreted as:

index key, foreign key (which it essentially is)
no confusion with Kubernetes' Name field
short and concise

duglin · 2017-10-13T23:04:19Z

I'm thinking of "cli" as "client" and less like "Command Line Interface", and with that view a nice UI fits too.

nilebox · 2017-10-14T08:20:08Z

I'm thinking of "cli" as "client"

call it ClientName then?

duglin · 2017-10-14T09:57:26Z

ok, made it "ClientName" even though I have a slight preference for "CliName" because its shorter. What do people think?

nilebox · 2017-10-14T10:13:44Z

LGTM.

nilebox · 2017-10-15T22:42:23Z

-				ExternalClusterServiceClassName: externalClusterServiceClassName,
-				ClusterServicePlanName:          externalClusterServicePlanName,
+				ClusterServiceClassClientName: clusterServiceClassClientName,
+				ClusterServicePlanName:        clusterServicePlanClientName,


shouldn't this plan name field in the PlanReference type have "ClientName" in its name as well?

I think ClusterServicePlanName refers to the k8s name. In that resource we now have both, ClusterServicePlanClientName and ClusterServicePlanName. I'm pretty sure it wouldn't be very kube-ish but making it ClusterServicePlanK8sName would clarify it.

yeah I got confused.

adding extra levels with grouping k8s- and client- names together would help, but I know you don't like extra nesting in YAML/JSON :)

duglin · 2017-10-18T20:45:58Z

On today's call we voted to use "ExternalName"

nilebox · 2017-10-19T00:18:27Z

revoking LGTM until renamed to "ExternalName".

duglin · 2017-10-19T04:22:10Z

ready for review - removing all LGTMs since it changed a lot since last time.

vaikas · 2017-10-19T15:58:46Z

Don't we need to update the *yaml files to these new values also?

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin · 2017-10-19T16:05:09Z

@vaikas-google I did just notice a few places I missed but none were in yaml files - which ones are you thinking of?

Signed-off-by: Doug Davis <dug@us.ibm.com>

vaikas · 2017-10-19T16:30:43Z

vaikas@vaikas-linux3:~/projects/go/src/github.com/kubernetes-incubator/service-catalog$ find contrib/ -type f -name '*yaml' -exec grep --with-filename externalClusterService {} ;
contrib/examples/walkthrough/ups-instance.yaml: externalClusterServiceClassName: user-provided-service
contrib/examples/walkthrough/ups-instance.yaml: externalClusterServicePlanName: default
contrib/examples/walkthrough/ups-instance-default-sp.yaml: externalClusterServiceClassName: user-provided-service
contrib/examples/apiserver/instance.yaml: externalClusterServiceClassName: test-serviceclass
contrib/examples/apiserver/instance.yaml: externalClusterServicePlanName: example-plan-1

duglin · 2017-10-19T16:32:04Z

those are part of the PR

vaikas · 2017-10-19T16:36:59Z

Oh, I'm sorry I missed them somehow :( Sorry for the noise.

pmorie

One nit which I will fix in a follow-up as I believe @duglin is traveling today.

pmorie · 2017-10-20T13:59:37Z

-	if old.Spec.ExternalClusterServicePlanName != new.Spec.ExternalClusterServicePlanName && new.Spec.ClusterServicePlanRef != nil {
-		errors = append(errors, field.Forbidden(field.NewPath("spec").Child("clusterServicePlanRef"), "clusterServicePlanRef must not be present when externalServicePlanName is being changed"))
+	if old.Spec.ClusterServicePlanExternalName != new.Spec.ClusterServicePlanExternalName && new.Spec.ClusterServicePlanRef != nil {
+		errors = append(errors, field.Forbidden(field.NewPath("spec").Child("clusterServicePlanRef"), "clusterServicePlanRef must not be present when servicePlanExternalName is being changed"))


Nit: servicePlanExternalName should be clusterServicePlanExternalName

I'm fine doing this in a follow-up

See: kubernetes-retired#1354 (review) Signed-off-by: Doug Davis <dug@us.ibm.com>

See: #1354 (review) Signed-off-by: Doug Davis <dug@us.ibm.com>

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2017

vaikas reviewed Oct 10, 2017

View reviewed changes

nilebox reviewed Oct 10, 2017

View reviewed changes

nilebox added the LGTM1 label Oct 10, 2017

pmorie added the do-not-merge label Oct 10, 2017

duglin force-pushed the removeExternal branch from b2246bc to 9d843f3 Compare October 10, 2017 16:45

vaikas added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2017

duglin force-pushed the removeExternal branch from 4116a8d to 8bcaa31 Compare October 14, 2017 01:25

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 Oct 14, 2017

duglin force-pushed the removeExternal branch from 8bcaa31 to cab11f7 Compare October 14, 2017 09:56

nilebox reviewed Oct 15, 2017

View reviewed changes

duglin force-pushed the removeExternal branch from cab11f7 to 70a3a53 Compare October 18, 2017 22:16

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

duglin changed the title ~~Remove "External" from some resource names~~ Move "External" around in some resource names/properties Oct 18, 2017

nilebox mentioned this pull request Oct 19, 2017

Rename ServiceInstanceSpec.External* #1425

Closed

nilebox removed the LGTM1 label Oct 19, 2017

move "External" to the middle of the property name

554a3bf

Signed-off-by: Doug Davis <dug@us.ibm.com>

generated files

fbcc270

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the removeExternal branch from 70a3a53 to fbcc270 Compare October 19, 2017 16:19

vaikas added LGTM1 and removed do-not-merge needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 19, 2017

pmorie reviewed Oct 20, 2017

View reviewed changes

pmorie added the LGTM2 label Oct 20, 2017

pmorie merged commit dff470f into kubernetes-retired:master Oct 20, 2017

duglin pushed a commit to duglin/service-catalog that referenced this pull request Oct 22, 2017

Fix typo from kubernetes-retired#1354

18f6316

See: kubernetes-retired#1354 (review) Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin mentioned this pull request Oct 22, 2017

Fix typo from #1354 #1456

Merged

nilebox pushed a commit that referenced this pull request Oct 22, 2017

Fix typo from #1354 (#1456)

40926cd

See: #1354 (review) Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin deleted the removeExternal branch August 2, 2018 20:11

Conversation

duglin commented Oct 10, 2017

Uh oh!

vaikas commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilebox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaikas commented Oct 10, 2017

Uh oh!

duglin commented Oct 10, 2017

Uh oh!

nilebox commented Oct 10, 2017

Uh oh!

nilebox commented Oct 10, 2017

Uh oh!

duglin commented Oct 10, 2017

Uh oh!

nilebox commented Oct 10, 2017

Uh oh!

duglin commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nilebox commented Oct 11, 2017

Uh oh!

vaikas commented Oct 11, 2017

Uh oh!

duglin commented Oct 11, 2017

Uh oh!

vaikas commented Oct 13, 2017

Uh oh!

nilebox commented Oct 13, 2017

Uh oh!

nilebox commented Oct 13, 2017

Uh oh!

duglin commented Oct 13, 2017

Uh oh!

nilebox commented Oct 14, 2017

Uh oh!

duglin commented Oct 14, 2017

Uh oh!

nilebox commented Oct 14, 2017

Uh oh!

nilebox Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Oct 18, 2017

Uh oh!

nilebox commented Oct 19, 2017

Uh oh!

duglin commented Oct 19, 2017

Uh oh!

vaikas commented Oct 19, 2017

Uh oh!

duglin commented Oct 19, 2017

Uh oh!

vaikas commented Oct 19, 2017

Uh oh!

duglin commented Oct 19, 2017

Uh oh!

vaikas commented Oct 19, 2017

duglin commented Oct 10, 2017 •

edited

Loading

nilebox Oct 15, 2017 •

edited

Loading