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 parent association in graph refresh #291

Merged
merged 2 commits into from
Sep 20, 2018
Merged

Fix parent association in graph refresh #291

merged 2 commits into from
Sep 20, 2018

Conversation

djberg96
Copy link
Collaborator

The when setting a parent, the parent_ems_ref helper method pulls its information straight from the JSON properties. Consequently, we need to explicitly downcase it to ensure that the association is set correctly.

@djberg96 djberg96 requested a review from Ladas September 20, 2018 18:46
@@ -72,7 +72,7 @@ def instances
series = persister.flavors.find(instance.properties.hardware_profile.vm_size.downcase)

rg_ems_ref = collector.get_resource_group_ems_ref(instance)
parent_ref = collector.parent_ems_ref(instance)
parent_ref = collector.parent_ems_ref(instance).try(:downcase)
Copy link
Member

Choose a reason for hiding this comment

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

@djberg96 are all possible parent ems_refs downcased? Could we just do this in parent_ems_ref? Want to avoid someone else using this method without knowing they need to downcase the result to be able to reference something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, we only did it for the managed images, not the unmanaged ones. I could modify it there, but then I would need to modify the parser again, plus add a data migration.

Copy link
Member

Choose a reason for hiding this comment

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

No that's fine, how about here ? Could we do .try(:image_reference).try(:id).try(:downcase)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that should work. I'll update it.

@miq-bot
Copy link
Member

miq-bot commented Sep 20, 2018

Checked commits https://github.com/djberg96/manageiq-providers-azure/compare/20b52b9d9b320005b035bff7c21a7c2aae54c137~...11930288795ef5d429effefaade96ac874147046 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@agrare agrare self-assigned this Sep 20, 2018
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.

LGTM

@agrare agrare merged commit baa5318 into ManageIQ:master Sep 20, 2018
@agrare agrare added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 20, 2018
@agrare agrare added the bug label Sep 20, 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.

3 participants