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

Create provider discovery feature. #61

Merged
merged 6 commits into from
Jul 27, 2017

Conversation

gabrielsvinha
Copy link
Contributor

@gabrielsvinha gabrielsvinha commented Jul 14, 2017

This Pull Request implement the discovery function for lenovo provider.

The idea is that, by receiving the ip + port, the function calls LXCA client in order to check if the params represents an appliance.

After that, it's created a new EMS instance and set all authentication with empty values. After the EMS in added, the user should update information via UI, then refreshing relationships.

Depends on: #66

@gabrielsvinha
Copy link
Contributor Author

@miq-bot add_label providers/physical-infrastructure, enhancement

@miq-bot
Copy link
Member

miq-bot commented Jul 14, 2017

@gabrielsvinha Cannot apply the following label because they are not recognized: providers/physical-infrastructure

@gabrielsvinha
Copy link
Contributor Author

@miq-bot assign @juliancheal

@gabrielsvinha gabrielsvinha reopened this Jul 18, 2017
@gabrielsvinha gabrielsvinha changed the title Create providert discovery feature. Create provider discovery feature. Jul 19, 2017
@miq-bot
Copy link
Member

miq-bot commented Jul 24, 2017

This pull request is not mergeable. Please rebase and repush.

@@ -73,7 +83,15 @@ def discover_queue(username, password, zone = nil)
private

def discover_from_queue(username, password)
discover(username, MiqPassword.decrypt(password))
discover(username, '')
Copy link
Member

Choose a reason for hiding this comment

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

Something's incongruous here 😄
Passing username and '' to a method expecting ip_address and port.

Seems like the caller of discover_queue should be passing all of username, password, ip_address, and port. Then, all of that should be queued up for this method, and all of those params passed to the discover method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. But I do not think neither username or password should be passed, don't you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this method is never called, should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

So, discover_queue puts discover_from_queue onto MiqQueue.

This will be called later, probably by a GenericWorker.

Currently, discovere_queue is receiving username and password, but I suspect it should also receive ip/hostname and port. In fact, it should at least receive ip/hostname and port.

Step back and think about the end user. They'll be presented with a page asking to discover an LXCA server. If they just enter username and password, the code would have to scan the entire network looking for LXCA servers, and attempting the username and password on each.

Instead, if the user is allowed to enter an ip and port, the code just has to attempt to connect to that ip and port and see if it's an LXCA server. If the user also supplies a username and password, the code can attempt to authenticate as well.

Another idea (something that's done for VMWare, I believe) is to allow the user to pass in an Subnet Mask. Then the code can search a range of IPs looking for LXCA servers. If you decide to do that, I think the username and password would not be needed. Since it's unlikely that if the code discovers two LXCA servers in that IP range, that they will both use the same credentials.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got your point, changes are on the way

@miq-bot
Copy link
Member

miq-bot commented Jul 27, 2017

Checked commits gabrielsvinha/manageiq-providers-lenovo@04dad9d~...9643024 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@juliancheal juliancheal merged commit 479fe4d into ManageIQ:master Jul 27, 2017
@bronaghs bronaghs added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 8, 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