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

Only look up resource constants directly available under the Krane module #720

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

camertron
Copy link
Contributor

@camertron camertron commented Jun 14, 2020

What are you trying to accomplish with this PR?

I ran into an interesting issue today I thought warranted a PR. I'm using kubedb to launch a Redis instance in my k8s cluster. In addition, I have a constant defined in my project called Redis.

The Redis kubedb resource looks something like this:

kind: Redis
metadata:
  name: foobar
  namespace: foobar-ns
spec:
  ... more stuff here

I tried to deploy using Krane and saw this in the list of discovered resources:

[INFO][2020-06-14 13:52:13 -0700]
[INFO][2020-06-14 13:52:13 -0700]	------------------------------------Phase 1: Initializing deploy------------------------------------
[INFO][2020-06-14 13:52:14 -0700]	All required parameters and files are present
[INFO][2020-06-14 13:52:14 -0700]	Discovering resources:
[INFO][2020-06-14 13:52:17 -0700]	  - <lots of resources>
[INFO][2020-06-14 13:52:17 -0700]	  - redis://127.0.0.1:6379/0
[INFO][2020-06-14 13:52:17 -0700]	  - <lots of resources>

Hmm, that's surprising. What's that Redis URL doing in there? The deploy then failed with this error:

ArgumentError: comparison of Redis with Krane::Secret failed

A little digging revealed this method in kubernetes_resource.rb:

def class_for_kind(kind)
  if Krane.const_defined?(kind)
    Krane.const_get(kind)
  end
rescue NameError
  nil
end

Interestingly, Krane.const_defined?('Redis') returns true. The documentation for const_defined? says it will check for the constant in the module's ancestors (including Object) unless a second false parameter is passed.

How is this accomplished?

Since I assume the intent was to look up constants under the Krane module directly, I added the second false param in this PR. Note that const_get works the same way.

What could go wrong?

Well, I suppose anyone who's relying on the old behavior, either implicitly or explicitly, is could experience a deploy failure. However in my opinion, this is a bug.

@camertron camertron requested a review from a team as a code owner June 14, 2020 21:01
@ghost ghost added the cla-needed label Jun 14, 2020
@camertron
Copy link
Contributor Author

Can someone re-run the CLA check for me?

@ghost ghost removed the cla-needed label Jun 15, 2020
Copy link
Contributor

@dturn dturn left a comment

Choose a reason for hiding this comment

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

I think you're correct and this behavior is a bug for people using krane as a library. Thanks for the fix!

@dturn dturn requested a review from a team June 15, 2020 23:58
Copy link
Contributor

@timothysmith0609 timothysmith0609 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, that one's pretty sneaky

@dturn dturn merged commit bf564d4 into Shopify:master Jun 16, 2020
@GustavoCaso GustavoCaso temporarily deployed to rubygems June 25, 2020 15:13 Inactive
@camertron camertron deleted the fix_const_lookup branch August 21, 2020 16:04
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.

4 participants