remove check for GUID regEx from plan externalid#1038
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
will this check to make sure its a non-empty string? |
|
@duglin yes, there is a test case for that I needed to fix. It looked like there was a copy/paste error on it from the previous test case so it wasn't working. https://github.com/kubernetes-incubator/service-catalog/pull/1038/files#diff-ab3c2e82db881a2a48465940cbe38df7R124 |
|
great - thanks!! will review later today |
| valid: true, | ||
| }, | ||
| { | ||
| name: "invalid serviceClass - missing plan guid", |
There was a problem hiding this comment.
how weird, it looks like the test name didn't match what was going on.
| } | ||
|
|
||
| for _, msg := range validateExternalID(plan.ExternalID) { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("externalID"), plan.ExternalID, msg)) |
There was a problem hiding this comment.
the presence check is above on line 110.
|
can you change |
| serviceClass: func() *servicecatalog.ServiceClass { | ||
| s := validServiceClass() | ||
| s.Plans[0].ExternalID = "40d-0983-1b89-★" | ||
| s.Plans[0].ExternalID = "" |
There was a problem hiding this comment.
isn't this a dup of the test on line 85?
There was a problem hiding this comment.
no, plan id vs class id
s := validServiceClass()
s.ExternalID = ""
s.Plans[0].ExternalID = ""
|
couple of minor comments, otherwise it looks good. |
emaildanwilson
force-pushed
the
removePlanIDGUIDCheck
branch
from
46f5809 to
8a347f2
Compare
test name guid -> id
emaildanwilson
force-pushed
the
removePlanIDGUIDCheck
branch
from
8a347f2 to
8ec4ade
Compare
|
@duglin Test references to |
|
My concern about removing the regex is that we might need to tighten it in the future, which isn't backward-compatible. I'd strongly prefer to just allow the period until we can get clarity on cloudfoundry/servicebroker#118 |
|
that would be breaking change in the spec which means v3 - which would break lots of other things too |
|
I think being spec compliant is more important right now |
|
Adding do-not-merge based on discussion in the July 20, 2017 meeting to relax the regex to allow periods and seek clarity on aspects of the OSB spec. |
|
Issue in OSB API: cloudfoundry/servicebroker#273 Been thinking about this and there are two uses of this ID that we need to think about: 1 - as the "name" that is exposed to a user. No matter what happens with cloudfoundry/servicebroker#273 I think that it would really be a mistake to expose this ID to the user as the way for them to specify which service/plan they want. The UX will just be too painful. So we really need to find a way to let people use the OSB API "name" field instead when specifying the service/plan. A proposal for how we might do this is here: #802 (comment) Even if we do hide these IDs from the user, using them as the "name" on the kube resource is still problematic because the allowable chars for a kube "name" is restricted to "lower case alphanumeric characters, -, and .". If OSB API does manage to restrict these fields such that kube can use them then we're fine. But, I have my doubts the spec can do that, so I think we may need to do some kind mapping like outlined in #802 (comment). |
|
Should we close this? |
|
Closing as suggested by @pmorie and no comments since January. |
|
looking at the status of the OSB API issue ( cloudfoundry/servicebroker#273 ) it appears that they decided they couldn't tighten the charset so I think this PR is still valid. Re-opening. |
k8s-ci-robot
added
the
size/S
|
@emaildanwilson want to rebase? |
|
We don't store plans in the service class anymore. is this relevant at all? Does it need to be reapplied to serviceplans specifically? |
MHBauer
added
needs-rebase
|
I am closing this because it was fixed in #1849 to allow periods in plan names. Feel free to open a new PR if there's still more to do! |
7 participants
The hashicorp/cf-vault-service-broker appends the plan name onto the GUID for the External ID using a period as the separator. The existing regEx validation does not allow for periods. The Pivotal docs do not specify an exact format and simply state
using a GUID is recommended.This change removes the regEx check in the api for Plan[].ExternalID so that any other brokers not following a strict GUID format will still work for k8s.