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

Tolerate lowercase kubenetes kinds #361

Merged
merged 1 commit into from
Nov 8, 2018
Merged

Conversation

f4tq
Copy link

@f4tq f4tq commented Nov 7, 2018

  • Istio stresses the type system by using unexpected kind definitions as explained in issue 360.

@f4tq
Copy link
Author

f4tq commented Nov 7, 2018

The tests that are failing also fail on a clean master.

Looks like it has to do with certs i.e test_config.rb:98.

@cben
Copy link
Collaborator

cben commented Nov 7, 2018

Thanks!
right, #359 is about the tests. I'd like to try fix them before merging any other changes.

@@ -138,7 +138,7 @@ def self.parse_definition(kind, name)
# See: https://github.com/kubernetes/kubernetes/issues/8115
kind = kind[0..-2] if %w[Endpoints SecurityContextConstraints].include?(kind)

prefix = kind[0..kind.rindex(/[A-Z]/)] # NetworkP
prefix = kind =~ /[A-Z]/ ? kind[0..kind.rindex(/[A-Z]/)] : kind # NetworkP
Copy link
Collaborator

Choose a reason for hiding this comment

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

This ternary assumes we'll see either kind: CamelCase or kind: lowercase, right?
What if we get kind: "lowerCamelCase"?
Also, even if using a lowercase kind as-is works, it feels pretty accidental given how we look for capital letters in prefix...

The purpose of this weird prefix dance is automatically adapting to the plural ending, which is sometimes s and sometimes es (and sometimes nothing e.g. Endpoints=endpoints but we special-case those above).

Except it doesn't extract just the s / esm[1] ends up with the whole lowercase end.
To take a fuller example:

kind = "OAuthAccessToken"
name = "oauthaccesstokens"
prefix = kind[0..kind.rindex(/[A-Z]/)]      #  "OAuthAccessT"
m = name.match(/^#{prefix.downcase}(.*)$/)  # #<MatchData "oauthaccesstokens" 1:"okens">
# Using new underscore algorithm from #355
underscore_entity(kind)                  # "o_auth_access_token"
underscore_entity(prefix)                # "o_auth_access_t"
underscore_entity(prefix) + m[1]         # "o_auth_access_tokens"

Can we rewrite the whole thing to work in a clearer way?

cc @eatwithforks

Copy link
Author

Choose a reason for hiding this comment

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

All the ternary does is check if there are any upper case letters. If there are, do the original action. If there aren't, then take kind as is.

This way you don't get a negative index which was causing an uncaught exception (and crash).

There are 2 tests. This fix allows any kind, irregardless of casing, to make it through.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. This is a strict improvement. I'll merge this and see if I can rewrite it clearer separately...

@cben
Copy link
Collaborator

cben commented Nov 8, 2018

close-cycting to re-run tests after #364 test fix.

@cben cben closed this Nov 8, 2018
@cben cben reopened this Nov 8, 2018
@cben cben changed the title tolerate lowercase kubenetes api s Tolerate lowercase kubenetes kinds Nov 8, 2018
@cben cben merged commit c864f8e into ManageIQ:master Nov 8, 2018
@cben cben added the bug label Nov 8, 2018
This was referenced Nov 17, 2018
cben added a commit to cben/kubeclient that referenced this pull request Nov 23, 2018
cben added a commit to cben/kubeclient that referenced this pull request Nov 26, 2018
@cben cben mentioned this pull request Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants