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

Lower case URLs #250

Merged
merged 1 commit into from
Jan 4, 2019
Merged

Lower case URLs #250

merged 1 commit into from
Jan 4, 2019

Conversation

fabianvf
Copy link
Member

This matches the behavior of kubectl, which is able to handle resources that improperly report their name camelCase. More investigation is required to verify that this doesn't break any other behavior (ie, is it safe to always lowercase the full url, or should only name be lowercased?)

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 14, 2018
mmazur added a commit to mmazur/containerized-data-importer that referenced this pull request Dec 18, 2018
(At least) by convention names and singularNames are all lower case. If
they're not, it might confuse client libraries that don't lowercase
everything proactively. Example:
openshift/openshift-restclient-python#250
@mmazur
Copy link
Member

mmazur commented Dec 18, 2018

I've tried looking around for this info and had no luck finding anything. While the API reference is quite explicit in enforcing CamelCase on kind, there's nothing on letter capitalization for name/singularName in API resource definitions. People are generally aware that the convention is to be lowercase, but there's no Authoritative Rule of Lowercasing that I could find.

If you're not sure how to proceed may I suggest limiting this patch to lowercasing only the resource names and shipping 0.8.2/0.9.0 like that? If it turns out somebody hits an issue down the line, you'll get a bug report and can always lowercase more of the url.

The issue for downstream here is that any kubevirt/CNV releases in existence have the resource definition for CDI's UploadTokenRequest borked. This is fixed in master, but that's gonna take a while to percolate downstream and in the mean time any code dependent on this library can't use CDI to its full extent. That'd be ok if I had the option of just adding a form of requires: openshift > 0.8.1 somewhere and be sure stuff will work despite CDI's issues, especially considering kubectl does this. Pretty please? :)

@pkliczewski FYI last paragraph ⬆️

@fabianvf
Copy link
Member Author

Yeah, the lower-case names are purely by convention (https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names), and are not explicitly enforced at the definition level. We'll definitely have some form of this fix included in the next release just to match the kubectl behavior.

mhenriks pushed a commit to kubevirt/containerized-data-importer that referenced this pull request Dec 20, 2018
(At least) by convention names and singularNames are all lower case. If
they're not, it might confuse client libraries that don't lowercase
everything proactively. Example:
openshift/openshift-restclient-python#250
@fabianvf
Copy link
Member Author

fabianvf commented Jan 3, 2019

/cherrypick release-0.8

@openshift-cherrypick-robot

@fabianvf: once the present PR merges, I will cherry-pick it on top of release-0.8 in a new PR and assign it to you.

In response to this:

/cherrypick release-0.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fabianvf fabianvf added this to the v0.8.2 milestone Jan 3, 2019
@fabianvf fabianvf merged commit 0750841 into openshift:master Jan 4, 2019
@openshift-cherrypick-robot

@fabianvf: new pull request created: #254

In response to this:

/cherrypick release-0.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fabianvf
Copy link
Member Author

fabianvf commented Jan 4, 2019

@mmazur this patch is in v0.8.2 , which is currently building and should be available in pypi later today.

willthames pushed a commit to willthames/openshift-restclient-python that referenced this pull request Feb 3, 2019
willthames pushed a commit to willthames/openshift-restclient-python that referenced this pull request Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants