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.

Sign up for GitHub

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

Jump to bottom

[WIP] Discover ip address of the physical infra provider #15553

Closed
wants to merge 2 commits into from
Closed

[WIP] Discover ip address of the physical infra provider #15553

wants to merge 2 commits into from

Conversation

CharlleDaniel
Copy link
Member

This PR is able to:

  • Discover the Ip address of the physical infra providers
    discover

  • After the refresh:
    discovery2

@CharlleDaniel CharlleDaniel force-pushed the discover_ip_physical_infra branch 2 times, most recently from 575b8da to d9d0ea6 Compare July 12, 2017 21:41
@CharlleDaniel CharlleDaniel changed the title Discover ip address of the physical infra provider [WIP] Discover ip address of the physical infra provider Jul 13, 2017
@miq-bot miq-bot added the wip label Jul 13, 2017
@miq-bot
Copy link
Member

miq-bot commented Jul 13, 2017

Checked commits CharlleDaniel/manageiq@e22aa0b~...dafee24 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@CharlleDaniel CharlleDaniel changed the title [WIP] Discover ip address of the physical infra provider Discover ip address of the physical infra provider Jul 13, 2017
@miq-bot miq-bot removed the wip label Jul 13, 2017
AndreyMenezes
AndreyMenezes reviewed Jul 13, 2017
View reviewed changes
app/models/ems_refresh/save_inventory_physical_infra.rb

def resolve_hostname(ipaddress, ems)
ems.hostname = "https://#{Resolv.getname(ipaddress)}/"
_log.info("EMS ID: #{ems.id}" + " Resolved hostname successfully.")
Copy link
Member

Choose a reason for hiding this comment

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

Can you put another print showing this method in action?

Copy link
Member Author

@CharlleDaniel CharlleDaniel Jul 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I can.

hostname

After the refresh:
hostname2

@AndreyMenezes
Copy link
Member

@blomquisg Can we merge this PR?

blomquisg
blomquisg reviewed Jul 26, 2017
View reviewed changes
app/models/ems_refresh/save_inventory_physical_infra.rb
end

def resolve_ip_address(hostname, ems)
ems.ipaddress = Resolv.getaddress(hostname)
Copy link
Member

Choose a reason for hiding this comment

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

@agrare you had a PR recently that changed the refresh process to treat the EMS as a first class item in inventory parsing, right?

I think that would work better here than doing more parsing in the save_inventory logic.

Copy link
Member

Choose a reason for hiding this comment

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

@blomquisg yes #15252 added the ability to update the ems in save_ems_inventory
And ManageIQ/manageiq-providers-vmware#62 is an (unmerged 😉 😉) example of its use.

Copy link
Member

Choose a reason for hiding this comment

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

ZING! (merged now)

Copy link
Member

Choose a reason for hiding this comment

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

🤣 thanks!

blomquisg
blomquisg suggested changes Jul 26, 2017
View reviewed changes
Copy link
Member

@blomquisg blomquisg left a comment

Choose a reason for hiding this comment

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

Let's move the parsing of the IP and Hostname for the EMS back into the inventory parsing code.

@agrare can point to the right place to do that.

@agrare
Copy link
Member

agrare commented Jul 26, 2017

@CharlleDaniel I would add something like parse_ems to your ems_inv_to_hashes here which sets the @data[:ems][:hostname] property, then save_ems_inventory should update it.

Fun fact you can update the :uid_ems and :api_version this way too which is what VMware and RHEV are doing.

@CharlleDaniel CharlleDaniel changed the title Discover ip address of the physical infra provider [WIP] Discover ip address of the physical infra provider Jul 28, 2017
@miq-bot miq-bot added the wip label Jul 28, 2017
@CharlleDaniel
Copy link
Member Author

Hi @blomquisg and @agrare, I agree with the sugestions, thanks for the review! I will work on it. 👍

@CharlleDaniel
Copy link
Member Author

@blomquisg, @agrare, @AndreyMenezes and @rodneyhbrown7 I close this PR because I open a new PR in the manageiq-provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blomquisg blomquisg blomquisg requested changes

@agrare agrare agrare left review comments

@AndreyMenezes AndreyMenezes AndreyMenezes approved these changes

Assignees

@blomquisg blomquisg

Labels
enhancement providers/physical wip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@CharlleDaniel @miq-bot @AndreyMenezes @agrare @blomquisg @chessbyte