-
Notifications
You must be signed in to change notification settings - Fork 141
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
Add PXE server credential validation endpoint #1017
base: master
Are you sure you want to change the base?
Conversation
@miq-bot cross-repo-tests ManageIQ/manageiq#21013 |
From Pull Request: ManageIQ/manageiq-api#1017
@@ -55,6 +55,13 @@ def edit_resource(type, id, data) | |||
end | |||
end | |||
|
|||
def verify_credentials_resource(_type, _id = nil, data = {}) | |||
task_id = PxeServer.verify_depot_settings_queue(User.current_user.userid, data) |
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.
@lfu if you're re-verifying credentials (e.g. if you edit an existing resource) we don't require the user to enter a password again but instead load it from vmdb if it doesn't exist (e.g. they only change the username or IP).
We do this by passing in data["id"] = id if id
here and handling it on the verify_depot_settings
side if an "id"
is passed.
I don't know if that's something that we want to do on the UI for PxeServers but I expect it probably is and would keep parity with provider authentication verification.
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.
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.
That's a good idea
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.
@lfu if you're re-verifying credentials (e.g. if you edit an existing resource) we don't require the user to enter a password again but instead load it from vmdb if it doesn't exist (e.g. they only change the username or IP).
@agrare verify_depot_settings
always creates a new instance to verify the settings.
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.
@lfu yes the problem with that is since we don't pull passwords back over the API if the user is editing an existing instance they have to re-type their password even if they don't intend on changing it.
We can handle that in a follow-up if you want but I don't think that's the best user experience.
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.
@agrare Do we have a similar pattern for this with provider validation you can point to? I think this probably is big enough that we should fix for this PR.
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.
Oh sorry ... didn't realize this was an outdated comment. Is this done then?
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.
Mostly asking because I don't see anything that looks up the existing instance on the core side's method - ManageIQ/manageiq#21013
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.
@Fryguy the API now matches the pattern for provider validation, core still doesn't lookup the instance yet not sure if you want that for this PR set or as a follow-up
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.
I think we need the core change then. Otherwise, we have a "broken" API in the interim.
9fec3d3
to
d4adfff
Compare
d4adfff
to
f9a04ac
Compare
Checked commit lfu@f9a04ac with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint app/controllers/api/pxe_servers_controller.rb
|
From Pull Request: ManageIQ/manageiq-api#1017
From Pull Request: ManageIQ/manageiq-api#1017
From Pull Request: ManageIQ/manageiq-api#1017
From Pull Request: ManageIQ/manageiq-api#1017
@miq-bot cross-repo-tests /all |
From Pull Request: ManageIQ/manageiq-api#1017
From Pull Request: ManageIQ/manageiq-api#1017
is this good to go? |
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 we are still waiting on #1017 (comment). This was mainly being introduced for UI changes, and I'm not sure if there's priority on it. @kavyanekkalapu would know better. Otherwise, we can just leave this open until we get back to that screen.
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
Looks like ManageIQ/manageiq#21013 has been merged. If we need changes, I can rebase/fixup. We were held up on #1017 (comment)
Ah, never mind. I mixed up the dates and thought that PR was merged after Nov 2021, but it was merged before |
From Pull Request: ManageIQ/manageiq-api#1017
From Pull Request: ManageIQ/manageiq-api#1017
From Pull Request: ManageIQ/manageiq-api#1017
From Pull Request: ManageIQ/manageiq-api#1017
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
This pull request has been automatically closed because it has not been updated for at least 3 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
3 similar comments
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
Depends on ManageIQ/manageiq#21013.
#926
@miq-bot add_label enhancement, kasparov/no