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
Host Verify Credentials connects directly to the host #6625
Comments
@agrare So, what's the alternative? I understand we need to run validate over the queue, I have no idea how to achieve that, or where to find any docs we have on that? (I mean, I assume there's a preferred way of queuing a verify operation that's documented somewhere?) |
Same as the rest of the operations we run over the queue, we need a verify_credentials_task method that calls verify_credentials. I can add that for you if you want. |
OK, I can see there is a And it looks like right now only ExtManagementSystem has it. So. if you wouldn't mind handling the backend bits, I'd be grateful :). Also, looks like the current code is relying on being able to catch exceptions, but it seems |
And one more thing, I'm worried about it using I don't think we can have our initial request wait 30 seconds... EDIT: separate issue though, providers do the same thing now |
Thanks! taking it from there :) |
Actually no, I can't. I still need to be able to access the exception on failure, ManageIQ/manageiq#19775 doesn't seem to address that. (+ we may need a change to account for #6667 (comment) #6667 (comment) ... that's a feature not a bug, and if validation on edit is supposed to work, we need an api that does not require the UI to know the password, and ensures we're validating the right thing (not reusing saved password for different server, for example).) |
@himdel ManageIQ/manageiq#19775 matches what we are doing with the new ems verify_credentials method not expected to be a drop-in replacement for what is currently there. You should be able to get the exception message from the task. |
@agrare Aah I think I see it now, thanks :)
it takes the same arguments as after some experimentation with the queue, seems like on error, So, seems like we only need to fix the queue runner to save the exception class somewhere, otherwise we have no way to handle (BTW as least BTW is there any wiki/doc document for these things? I think I'll start one if not, too many undocumented core things to remember :). |
This is true for all |
Hm that's going to be tough because Is there another way to do this? Can we catch that exception and return a known string that you can key off of? Or just drop the weird exception handling from the UI completely? |
Really? That's not what I'm seeing
|
For which things? A lot of this is insider developer stuff which unless it is really obscure I think we expect people to be able to figure out. If there are things you think would benefit from documentation go for it though! |
I mean.. yes and no. If using the queue means we can't handle exceptions, then we can't use the queue in general. That's really it :). I would not treat exception handling using the exception class as "weird" ;), that should be the default. As for the specific case ... maybe? But sounds like a lot of work in the wrong direction. But, we can also catch the exception inside the |
I was testing with... diff --git a/app/models/miq_task.rb b/app/models/miq_task.rb
index 3afbfd1cf6..5f607f4aa7 100644
--- a/app/models/miq_task.rb
+++ b/app/models/miq_task.rb
@@ -64,6 +64,10 @@ class MiqTask < ApplicationRecord
scope :no_status_selected, -> { running.where.not(:status => %(Ok Error Warn)) }
scope :with_status_in, ->(s, *rest) { rest.reduce(MiqTask.send(s)) { |chain, r| chain.or(MiqTask.send(r)) } }
+ def magic
+ raise NoMethodError, "foo"
+ end
+
def ensure_started
self.started_on ||= Time.now.utc if state == STATE_ACTIVE
end
but testing again on a different machine yields the result you're seeing ¯\_(ツ)_/¯. I probably missed an unrealted thing, sorry :). |
Well, honestly any of them :) In the ideal case, I would say the UI should never use any backend method that's not documented in developer docs. That's the only way to end up with a clean, documented interface down the road. But yeah, we're not there yet, so I'll start writing down notes in https://github.com/ManageIQ/manageiq-ui-classic/wiki/Backend-knowledge-tidbits :) |
We have to use the queue for ems_operations, that isn't an option this is a bug. I'm happy to add in the exception class to the error string if that is what you need but not using the queue isn't an option. |
Reading back, I think we need to consider the long term case where everything UI goes over the REST API. If we were to implement this via API, you wouldn't get exceptions raised. Instead I guess it would be part of the miq_task payload. So, I would lean towards a design more in that vein, and not use exception handling. |
OK, so we need to move the SSH key logic to the side that is executing the task on the queue, and figure out a way of sending a flash message contents. My problem is that right now as it is, we're simply losing the information. We have no way to implement the same behaviour without knowing the exception class, the message is not useful. |
I can add an option to the verify task to accept pubkey changes if you add a checkbox for it on the UI, the flash message can display the error which should mention the error was that the public key of the server has changed |
@agrare I'm not sure how that would work, if the checkbox is off, all we could do is display a generic "Too bad" message, because we have no way of knowing that was the issue, right? If we want to preserve the existing behaviour, the task has to fail in a way that can tell us what happened and allow us to queue again. If we're OK with a "Failed" message and a "force" checkbox, I can do that :). |
Won't you be able to show the exception message from the task? |
Oh, if you change the task to use the right exception message, sure :). But then that task needs to catch I still think the UI should be outputting the message, based on the task data (such as the exception, or a more explicit way of passing the information, like a result hash with a |
((Note that if the backend is outputting the message, it needs to figure out the right language, not sure we have i18n set up in the queue?)) ((Or use the N_ + __ combo, but the message needs to be fully static then.)) |
@agrare @kavyanekkalapu can someone from the UI team bring this across the finish line. Obviously, there is no rush, but would be good for more maintainable code |
This issue has been automatically marked as stale because it has not been updated for at least 3 months. If you can still reproduce this issue on the current release or on Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
When verifying host credentials the verify operation is run from the controller directly not over the queue: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/host_controller.rb#L229
The text was updated successfully, but these errors were encountered: