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

make api_service method work #355

Merged
merged 1 commit into from
Mar 10, 2020
Merged

Conversation

eatwithforks
Copy link
Contributor

@cben
Copy link
Collaborator

cben commented Oct 9, 2018

Thanks!

Let's see all resources with consecutive capital letters:

github.com/cben/kubernetes-discovery-samples$ ag -o '"kind":.*[A-Z][A-Z]\S*' --nonumbers --nofilename */api*/ | sort -u

"kind": "APIGroup",
"kind": "APIGroupList",
"kind": "APIResourceList",
"kind": "APIService",
"kind": "APIVersions",
"kind": "OAuthAccessToken",
"kind": "OAuthAuthorizeToken",
"kind": "OAuthClient",
"kind": "OAuthClientAuthorization",

(APIGroup, APIGroupList, APIResourceList are not actual API resources, they're kinds of the discovery results themselves)

kind old method names new method names
APIService apiservice api_service
APIVersions apiversions api_versions
OAuthAccessToken oauth_access_token o_auth_access_token
OAuthAuthorizeToken oauth_authorize_token o_auth_authorize_token
OAuthClient oauth_client o_auth_client
OAuthClientAuthorization oauth_client_authorization o_auth_client_authorization

and couple more example, one made up, one from istio CRD referenced in #361:

kind old method names new method names
lowerCamelUPPERCase lower_camel_uppercase lower_camel_upper_case
HTTPAPISpecBinding httpapispec_binding httpapi_spec_binding

Hmm, is this a breaking change? I agree it's better (and matches Rails' .underscore behaviour) 👍, but perhaps someone already has working code with say client.get_apiservice and now will need to change to client.get_api_service...
BTW, many of these kinds are recent but some go back quite a while, APIService appeared in kube 1.7...

  • I definitely don't want to release this in a patch bump.
  • compromise on calling this a bug but releasing in a minor bump?
  • major bump?

@grosser
Copy link
Contributor

grosser commented Oct 9, 2018 via email

@cben
Copy link
Collaborator

cben commented Nov 3, 2018

I'm working on making both old and new work...

@cben cben mentioned this pull request Nov 5, 2018
@cben
Copy link
Collaborator

cben commented Nov 16, 2018

Rebased, still blocked on compatibility, working on it.
I'm going to first rewrite parse_definition and add more tests in separate PR and then return to this...

Changes ABBREVIATIONCamelCase treatment to recognize end of
ABBREVIATION as word boundary.
Similar to ActiveSupport `String#underscore` behavior.
@cben
Copy link
Collaborator

cben commented Dec 27, 2018

Rebased again.
But with recent PRs inflating the whole (kind,name)->method names derivation a lot, I'm not sure I want to complicate it further by computing computing both old and new names for compatibility :-(

@cben
Copy link
Collaborator

cben commented Apr 4, 2019

reminder to self: there will never be a perfect time but fixing this sooner is better.
I should merge this and bump major version sometime soon...

@cben cben merged commit 5737370 into ManageIQ:master Mar 10, 2020
@cben cben mentioned this pull request Mar 10, 2020
11 tasks
@cben
Copy link
Collaborator

cben commented Mar 10, 2020

Oh, wow. Sorry for holding this up so long.
I'm planning a major version bump soon, see #435

cben added a commit to cben/kubeclient that referenced this pull request Jun 14, 2020
These changes were backported to v4.y branch
(all from master except ManageIQ#355).
Planning to also merge this to master.
cben added a commit to cben/kubeclient that referenced this pull request Jun 14, 2020
These changes were backported to v4.y branch
(all from master except ManageIQ#355).
Planning to also merge this to master.
@cben cben mentioned this pull request Jun 14, 2020
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.

3 participants