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

Bug 1247680 - must not truncate svc names in the cli, rely on API validation #4220

Merged
merged 1 commit into from
Aug 17, 2015

Conversation

fabianofranz
Copy link
Member

Bug 1251601 and 1247680.

This will now display the error message from the API instead of silently truncating and lowercasing the provided names:

The Service "aaaaabbbbbcccccdddddeeeee" is invalid.

* metadata.name: invalid value 'aaaaabbbbbcccccdddddeeeee', Details: must be a DNS 952 label (at most 24 characters, matching regex [a-z]([-a-z0-9]*[a-z0-9])?): e.g. "my-name"

@fabianofranz
Copy link
Member Author

@jwforres ptal

@jwforres
Copy link
Member

worth adding a testcase for? otherwise LGTM

@fabianofranz
Copy link
Member Author

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3005/) (Image: devenv-fedora_2173)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to f94c655

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/4212/)

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to f94c655

openshift-bot pushed a commit that referenced this pull request Aug 17, 2015
@openshift-bot openshift-bot merged commit d64e946 into openshift:master Aug 17, 2015
@liggitt
Copy link
Contributor

liggitt commented Aug 18, 2015

people who comment on pull requests post-merge are the worst. also, will a name longer than 24 characters leave a bunch of partially created resources when it fails on services?

@liggitt
Copy link
Contributor

liggitt commented Aug 18, 2015

@smarterclayton comment on what you want for bug 1251601 and 1247680.

@smarterclayton
Copy link
Contributor

Danger, this is not safe. We have to restrict --name, but this is the
service name which we infer from another input (like the git repo).

So this needs to be reverted - we need to validate

  • that --name fits the minimum bar for a valid value
  • that anything we infer from a Git repo name or Image name is of the
    proper lengths.

So we have to truncate this in the CLI.

On Tue, Aug 18, 2015 at 4:39 PM, Jordan Liggitt notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton comment on what you
want for bug 1251601 and 1247680.


Reply to this email directly or view it on GitHub
#4220 (comment).

Clayton Coleman | Lead Engineer, OpenShift

@liggitt
Copy link
Contributor

liggitt commented Aug 18, 2015

and lowercase?

@fabianofranz
Copy link
Member Author

@liggitt hrm, I'm afraid so, you are correct.

@jwforres
Copy link
Member

It also has to start with a letter, see
https://bugzilla.redhat.com/show_bug.cgi?id=1251845

On Tue, Aug 18, 2015 at 4:42 PM, Jordan Liggitt notifications@github.com
wrote:

and lowercase?


Reply to this email directly or view it on GitHub
#4220 (comment).

@jwforres
Copy link
Member

FYI, fix for the console #4249

@fabianofranz
Copy link
Member Author

So we are replicating the validation that happens in the API on every client? Shouldn't we rely on the API for validation, but have it fail on (some) errors, like validation?

@fabianofranz
Copy link
Member Author

Reverted and properly fixed for --name in #4251.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants