-
Notifications
You must be signed in to change notification settings - Fork 382
Remove regex validation of service class external IDs #2303
Remove regex validation of service class external IDs #2303
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl looks good.
@@ -248,10 +257,10 @@ func TestValidateServiceClass(t *testing.T) { | |||
valid: false, | |||
}, | |||
{ | |||
name: "invalid serviceClass - invalid guid", | |||
name: "invalid serviceClass - invalid externalID", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dup of test immediately above.
serviceClass: func() *servicecatalog.ClusterServiceClass { | ||
s := validClusterServiceClass() | ||
s.Spec.ExternalID = "1234-4354a\\%-49b" | ||
s.Spec.ExternalID = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dup of test immediately above.
@@ -62,14 +62,23 @@ func TestValidateClusterServiceClass(t *testing.T) { | |||
valid: true, | |||
}, | |||
{ | |||
name: "valid serviceClass - period in GUID", | |||
name: "valid serviceClass - period in externalID:2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the :2
for?
serviceClass: func() *servicecatalog.ClusterServiceClass { | ||
s := validClusterServiceClass() | ||
s.Spec.ExternalID = "4315f5e1-0139-4ecf-9706-9df0aff33e5a.plan-name" | ||
return s | ||
}(), | ||
valid: true, | ||
}, | ||
{ | ||
name: "valid serviceClass - underscore in ExternalID", | ||
serviceClass: func() *servicecatalog.ClusterServiceClass { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also need this test below for ServiceClass, (in theory)
ec31964
to
bf26320
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/assign @duglin |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: duglin 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:
Approvers can indicate their approval by writing |
Per the OSB API spec, although external ID is recommended to be a GUID, it can be any non-empty string. So, remove the regex validation, and add a validation to assert it's non-empty.