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

Fix bug in InventoryCollection#find_by with non-default ref #15648

Merged
merged 1 commit into from
Jul 25, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented Jul 25, 2017

Small followup to #15447.
The ref was not being passed to find(), so the id was looked up in wrong hash, most likely resulting in nil.
(lazy_find_by(...) with same params worked.)
At this point, no code was affected (containers graph refresh is only use of secondary refs, but only used lazy_find_by; I stumbled on this when debugging interactively...).

@miq-bot add-label bug

@agrare @Ladas Please review. (This could be a good time to ask me to add tests ;-))

@miq-bot miq-bot added the bug label Jul 25, 2017
The ref was not being passed to find() and result was nil.
(lazy_find().load with same params worked.)
@cben cben force-pushed the fix-InventoryCollection-find_by-ref branch from 52734ec to 1359023 Compare July 25, 2017 12:59
@miq-bot
Copy link
Member

miq-bot commented Jul 25, 2017

Checked commit cben@1359023 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

@cben looks great 👍

yeah, the specs would be nice, but we can wait till we finish the secondary indexes(db loading, to_yaml, from_yaml, etc.). Then we should test also the targeted approach.

So, I am fine with complex tests a bit later, rather than small one now. :-) @agrare ?

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

Yeah this is a small enough fix I'm good with getting it in now

@agrare agrare merged commit 7e1997a into ManageIQ:master Jul 25, 2017
@agrare agrare added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants