Skip to content
This repository was archived by the owner on May 6, 2022. It is now read-only.

Support for OSB PR #452 to allow periods in the name field#1849

Merged
duglin merged 1 commit into
kubernetes-retired:masterfrom
amshuman-kr:osb-pr-452-support
Mar 19, 2018
Merged

Support for OSB PR #452 to allow periods in the name field#1849
duglin merged 1 commit into
kubernetes-retired:masterfrom
amshuman-kr:osb-pr-452-support

Conversation

@amshuman-kr
Copy link
Copy Markdown
Contributor

Support for OSB PR #452 which was merged recently.

I have added/modified some unit tests.

No integration tests were introduced as part of this change but I have executed the current integration tests successfully on my local machine.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 19, 2018
Copy link
Copy Markdown
Contributor

@nilebox nilebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

)

const serviceClassNameFmt string = `[-a-zA-Z0-9]+`
const serviceClassNameFmt string = `[-.a-zA-Z0-9]+`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is compatible with Kubernetes names, see https://kubernetes.io/docs/concepts/overview/working-with-objects/names/

By convention, the names of Kubernetes resources should be up to maximum length of 253 characters and consist of lower case alphanumeric characters, -, and ., but certain resources have more specific restrictions.

Even though it's not critical now, it might be important in the future.

Copy link
Copy Markdown
Contributor Author

@amshuman-kr amshuman-kr Mar 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nilebox Thanks for the review!

This change brings service catalog up-to-speed on the OSB PR#452 to support period ('.') characters in service brokers' service class/plan names.

This helps in integrating some existing service brokers which use the '.' characters in the names without having to the change the brokers' catalog (which might cause some migration problems).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amshuman-kr yeah it's all good, I just left the comment for other reviewers :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to escape the . so its not "any char" ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

. is considered a normal character inside regex character classes. So, no escaping is required.

Just to be sure, I have added a test case where _ (underscore) is detected as an invalid character.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great thanks

@nilebox nilebox added the LGTM1 label Mar 19, 2018
@duglin
Copy link
Copy Markdown
Contributor

duglin commented Mar 19, 2018

LGTM

@duglin duglin added the LGTM2 label Mar 19, 2018
@duglin duglin merged commit 93dab13 into kubernetes-retired:master Mar 19, 2018
@amshuman-kr amshuman-kr deleted the osb-pr-452-support branch March 19, 2018 09:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants