-
Notifications
You must be signed in to change notification settings - Fork 91
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
Refactors ansible credentials to be more generic, override with dialog input #1417
Refactors ansible credentials to be more generic, override with dialog input #1417
Conversation
cc @kedark3 gonna wanta work with you to ensure this works as intended before merging (but maybe tomorrow 😏 ) |
Also displays any config_info credential object with 'credential' in the key
8ba1200
to
3e6aae1
Compare
Checked commit AllenBW@3e6aae1 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
@AllenBW let me know if you want me to review. Happy to help! |
function normalizeDialogKeys (key) { | ||
const normalizedKey = key.replace(/dialog_/i, '') | ||
|
||
return normalizedKey === 'credential' ? 'credential_id' : normalizedKey |
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.
To include other credential type, should it be something like
return normalizedKey.contains('credential') ? normalizedKey+'_id' : normalizedKey
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.
@bzwei if at dialog has a credential override of dialog_cloud_credential_id
the change you are proposing would result in a normalized key of cloud_credential_id_id
This conditional is to catch the edge case of dialog_credential
which would be normalized to credential
which would not override the desired machine credential which is called credential_id
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.
per our discussion, we wanted a generic solution for future dialog overriding credentials.
I am OK (and good to play it safely) if we just do the correction for credential
alone here, and add more corrections in the future if other other credential types can be overridden. (currently only (machine) credential can be overridden)
Since you only need to look for one and only credential
, you do not need to use a map.
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.
Since you only need to look for one and only credential, you do not need to use a map.
unclear watcha mean by this ☝️ 😕
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 thought you use a map from credential
to value. In this map it can be only one pair. why bother?
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.
In the case of provision
and dialog
present, we map through each dialog
keys, normalize them and assign them to the config_info object. Without this we miss all overrides 😬
@kedark3 was gracious enough to make a machine, looks like we're lookin' gooooood!!! 🌮 💃 |
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.
LGTM 👍
Refactors ansible credentials to be more generic, override with dialog input (cherry picked from commit 9eeaab5) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1565160
Gaprindashvili backport details:
|
Also displays any config_info credential object with 'credential' in the key Fine dup for: ManageIQ#1417
Also displays any config_info credential object with 'credential' in the key Fine dup for: ManageIQ#1417
Backported to Fine via #1418 |
fixes https://bugzilla.redhat.com/show_bug.cgi?id=1557504
fixes https://bugzilla.redhat.com/show_bug.cgi?id=1540689
Also displays any config_info credential object with 'credential' in the key
This work follows the following assumptions:
cc @bzwei - yer a flippin 🧙♂️ ❤️