-
Notifications
You must be signed in to change notification settings - Fork 358
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
Validate cloud credentials on the queue #1580
Conversation
@jntullo Is this only for "new" providers? I think so, but am just asking because authentication_check not only checks the creds work but also marks the authentication record as valid... so calling I like this approach. The more we can do async out of the UI workers, the less junk we need to load in them and bloat these processes. |
I think you can still have the one method if you use |
[params[:project], params[:service_account], {:service => "compute"}] | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good opportunity for case/when. Also, should there be an "else" condition?
776b676
to
176c8ca
Compare
[params[:project], params[:service_account], {:service => "compute"}] | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to clean it up a bit like so:
case ems
when ManageIQ::Providers::Openstack::CloudManager
# stuff
when ManageIQ::Providers::Amazon::CloudManager
# stuff
end
And so on. Ruby uses ===
under the hood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djberg96 there was something up with my environment when this was WIP. even through the console, comparing ManageIQ::Providers::Openstack::CloudManager === ManageIQ::Providers::Openstack::CloudManager
was returning false. Not sure what eventually happened to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jntullo hmm...well, if the specs are passing and whatever local UAT you're doing works, it should be fine.
c6a020d
to
5af3176
Compare
Note: There is a separate BZ involving infra providers, so I will be creating a separate PR for those because it will involve more PRs to different provider repos. This is cloud only. @miq-bot remove_label wip |
@miq-bot add_label wip |
5af3176
to
41d416b
Compare
@jntullo is this for validation from a cloud provider add/edit form? If so, the UI is waiting (synchronously) on the response from validation before it will enable the Save button. Unless we change the front end, I think this is a synchronous UI task (i.e. the UI is blocked until done). Normally, when the UI code puts a task on the queue, we use a "wait_for_task" method that allows the UI to check occasionally to see if the task is finished, thus freeing up the UI thread. Should we be changing the front end code to use that implementation? Adding @AparnaKarve as she is more familiar with how this all works. |
@jntullo Can you look up some examples of And if not, do you think most of the code in |
ea07327
to
5db35d0
Compare
@jntullo That would be my best guess too. I did a bit of digging around and found that |
5db35d0
to
9832bca
Compare
9832bca
to
3a76c63
Compare
Checked commits jntullo/manageiq-ui-classic@252f251~...3a76c63 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@@ -59,14 +59,40 @@ def update_ems_button_save | |||
end | |||
end | |||
|
|||
def update_ems_button_validate(verify_ems = nil) | |||
def update_ems_button_validate | |||
result, details = realtime_authentication_check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jntullo we always call realtime_authentication_check
when editing existing provider, wondering if we need to check for session[:selected_roles].try(:include?, 'user_interface') here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h-kataria hm, I am not sure if we need it here because the zone isn't considered with realtime_authentication_check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than that one concern in update_ems_button_validate method, changes look good to me, @AparnaKarve do you know if it's ok to always call realtime_authentication_check when editing exiting EMS
@h-kataria I do not know the answer to that. |
@jntullo fine/yes ? |
@miq-bot add_label fine/no |
### This is very WIPThis PR is for a BZ related to provider validation not occurring in the specified zone and is related to the closed PR ManageIQ/manageiq#4869 that began to work on validating credentials in the queue (I stole a bit of it, thanks @agrare!)
I started to address some of the concerns from @Fryguy's comment by doing the following:
raw_connect
class methodThere are still a couple of things that need to be done for this to work completely:
*raw_connect
foramazon
andvmware
need to be modified slightly to actually validate / not just pass back a connection (have been playing around with this locally to test this PR)* need to encrypt the credentials before putting into the queue* write specs* fix whatever I inevitably broke 😛Before I continue down this 🐰 hole, I wanted to start a discussion with this PR to see if there are other ideas and/or concerns about this approach.
cc: @jrafanie @blomquisg