Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate entity names locally #3296

Merged
merged 3 commits into from
Jan 8, 2019
Merged

Validate entity names locally #3296

merged 3 commits into from
Jan 8, 2019

Conversation

richard-cox
Copy link
Contributor

- service instances, route host and route path
- app, org and space seem to handle long names fine
@cfdreddbot
Copy link

✅ Hey richard-cox! The commit authors and yourself have already signed the CLA.

@richard-cox richard-cox self-assigned this Jan 2, 2019
@codecov
Copy link

codecov bot commented Jan 2, 2019

Codecov Report

Merging #3296 into v2-master will decrease coverage by 0.01%.
The diff coverage is 60%.

@@              Coverage Diff              @@
##           v2-master    #3296      +/-   ##
=============================================
- Coverage      70.77%   70.75%   -0.02%     
=============================================
  Files            642      644       +2     
  Lines          28358    28474     +116     
  Branches        6447     6461      +14     
=============================================
+ Hits           20071    20148      +77     
- Misses          8287     8326      +39

Copy link
Contributor

@KlapTrap KlapTrap left a comment

Choose a reason for hiding this comment

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

We should truncate the application name we use to pre-populate the host on step two of the application creation so it's within the host char limit.

@KlapTrap KlapTrap added needs attention This PR needs attention and removed ready for review labels Jan 4, 2019
@richard-cox
Copy link
Contributor Author

@KlapTrap I thought about that, but as the change would not be visible the user would end up with an outcome they weren't expecting. The other alternative would be to fire the validation on change rather than defocus

@richard-cox richard-cox added ready for review and removed needs attention This PR needs attention labels Jan 4, 2019
@KlapTrap
Copy link
Contributor

KlapTrap commented Jan 4, 2019

I'm happy with either.

- template form failed to do this properly, converted to reactive
- there's some odd code in here, it also fails when creating tcp routes (unrelated)
- this whole step should be replaced with the create route stepper when merged into deploy
@richard-cox
Copy link
Contributor Author

@KlapTrap Cool, I've made it so the form shows validation errors on enter

Copy link
Contributor

@nwmac nwmac left a comment

Choose a reason for hiding this comment

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

LGTM

@richard-cox richard-cox added needs attention This PR needs attention and removed ready for review labels Jan 7, 2019
@richard-cox richard-cox added ready for review and removed needs attention This PR needs attention labels Jan 7, 2019
Copy link
Contributor

@nwmac nwmac left a comment

Choose a reason for hiding this comment

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

LGTM

@nwmac nwmac merged commit b46a4bf into v2-master Jan 8, 2019
@nwmac nwmac deleted the limit-service-instance-name branch January 8, 2019 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service instance names should be limited to 50 chars
4 participants