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

Drop legacy Host credentials payload, replaced by DDF params #1244

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

agrare
Copy link
Member

@agrare agrare commented Dec 19, 2023

For a period of time we supported both the older :credentials payload as well as the newer DDF authentications => {auth_type => {userid, password}} style.

#1202

Now that the host edit form is in react (ManageIQ/manageiq-ui-classic#8608) we no longer need to keep this old style around.

Depends on:

Follow-up:

  • Update the manageiq-documentation/api/references to udpate the host examples

@agrare agrare requested a review from bdunne as a code owner December 19, 2023 20:21
@agrare agrare force-pushed the drop_legacy_hosts_edit_verify branch from b33208b to 3f9e2a8 Compare December 19, 2023 20:22
creds.reverse_merge!(:userid => host.authentication_userid(auth_type))
hash[auth_type.to_sym] = creds
if authentications.present?
authentications.deep_symbolize_keys!
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE update_authentications only works with symbolized keys

remember_host = data["remember_host"] == "true"
authentications, auth_type = symbolize_password_keys!(data[AUTH_ATTR])
{:task_id => host.verify_credentials_task(User.current_userid, auth_type, :credentials => authentications, :remember_host => remember_host)}
auth_type = data["authentications"].keys.first
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE In the future I'd like to make Host#verify_credentials as close as possible to ExtManagementSystem#verify_credentials which takes just the data payload and no auth_type.

spec/requests/hosts_spec.rb Outdated Show resolved Hide resolved
spec/requests/hosts_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor question in the specs.

@agrare agrare force-pushed the drop_legacy_hosts_edit_verify branch from 3f9e2a8 to 6edb2a3 Compare December 20, 2023 13:04
@miq-bot
Copy link
Member

miq-bot commented Dec 20, 2023

Checked commit agrare@6edb2a3 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@agrare agrare changed the title Drop legacy Host credentials payload, replaced by DDF params [WIP] Drop legacy Host credentials payload, replaced by DDF params Dec 20, 2023
@miq-bot miq-bot added the wip label Dec 20, 2023
@agrare agrare changed the title [WIP] Drop legacy Host credentials payload, replaced by DDF params Drop legacy Host credentials payload, replaced by DDF params Dec 20, 2023
@miq-bot miq-bot removed the wip label Dec 20, 2023
@kbrock kbrock self-assigned this Jan 2, 2024
@kbrock kbrock added the cleanup label Jan 2, 2024
@kbrock kbrock merged commit 3e15b0c into ManageIQ:master Jan 2, 2024
3 checks passed
@agrare agrare deleted the drop_legacy_hosts_edit_verify branch January 2, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants