raw_connect method for infra provider #15914
Conversation
jntullo
changed the title
| @@ -42,4 +41,38 @@ def with_provider_connection(options = {}) | |||
| vim.try(:disconnect) rescue nil | |||
| end | |||
| end | |||
|
|
|||
| module ClassMethods | |||
| def make_connection(options) | |||
There was a problem hiding this comment.
Should this be a private method? EDIT: nvm I didn't see it used at the end of connect
| end | ||
| end | ||
|
|
||
| def raw_connect(options) |
There was a problem hiding this comment.
raw_connect for the other providers typically does not verify the connection. It's purpose is closer to your make_connection method. connect and validate are usually implemented by calling raw_connect to get the native handle.
|
@agrare Aside, doesn't this mixin belong in manageiq-providers-vmware? |
Yes it is on my list of things to move, it is used by |
|
@jntullo not sure but I think |
|
@agrare @Fryguy I have been ensuring other raw connects do the validations - you can see all of the related PRs / explanation here: ManageIQ/manageiq-ui-classic#1580 |
|
@jntullo maybe it'd be better to add a class level |
|
@agrare not a bad idea, however I have already made my rounds through almost every provider to get this validation on the queue moving. Would it be okay to do a follow up to change it to a |
|
I agree with @agrare . raw_connect was always the lowest level way to get a handle as a pure function (so just pass args, and get a handle), and as such it's useful for diagnostic purposes from rails console for example. I see validation as a layer above that, so it should a separate method that uses raw_connect, IMO. |
|
I would be ok with followups if it's going to be a pain to "undo" the changes in a number of places, but I'll leave it up to @agrare |
|
if this whole validation on the queue didn't already involve > 15 PRs I would have no problem with this change :) |
|
I'm okay with a temporary solution but can you leave the existing We do some specific exception handling in the VMware code to handle re-connects and I don't want the validate exception handlers messing with the exceptions being raised. |
| options[:user] ||= authentication_userid(options[:auth_type]) | ||
| options[:pass] ||= authentication_password(options[:auth_type]) | ||
|
|
||
| self.class.make_connection(options) |
There was a problem hiding this comment.
So lets leave this the way it currently is, we'll have two similar code paths for connect but when you do your followup to split raw_connect and validate we can move this over to raw_connect
do not change the connect method
|
Checked commit jntullo@e5a29e8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/models/mixins/vim_connect_mixin.rb
|
| if options[:fault_tolerant] | ||
| MiqFaultTolerantVim.new(options) | ||
| else | ||
| MiqVim.new(options[:ip], options[:user],options[:pass]) |
There was a problem hiding this comment.
Can you fix the missing space after the comma here in a follow-up PR?
|
@jntullo is ManageIQ/manageiq-providers-vmware#104 ready to be merged after this is in? |
|
@agrare yup it will be! thanks! 👍 |
Adding a
raw_connectclass method for the Vmware Infra Provider that will validate the credentials. Ensuring reuse of existing code while not changing current functionalityrelated: ManageIQ/manageiq-providers-vmware#104
Rationale for needing
raw_connectmethod can be found: ManageIQ/manageiq-ui-classic#1580 (will be working on similar update for infra and container providers)@miq-bot add_label wip, providers, enhancement