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

Refactors ansible credentials to be more generic, override with dialog input #1417

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

AllenBW
Copy link
Member

@AllenBW AllenBW commented Apr 3, 2018

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:

  1. dialog objet will always over config info - for provision does not exist in reirement
  2. dialog_credientail will always override credential_id
  3. from the dialog object, to the config_info - dialog_cloud_credential -> (normalized) cloud_credential - will override cloud_credential
  4. when avaliable, all possible credential types will have "credentail" in the object key, and be contained in config info (if not overridden)

cc @bzwei - yer a flippin 🧙‍♂️ ❤️

@AllenBW
Copy link
Member Author

AllenBW commented Apr 3, 2018

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
@AllenBW AllenBW force-pushed the BZ/#1557504-ansible-credentails branch from 8ba1200 to 3e6aae1 Compare April 4, 2018 12:46
@miq-bot
Copy link
Member

miq-bot commented Apr 4, 2018

Checked commit AllenBW@3e6aae1 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@kedark3
Copy link

kedark3 commented Apr 4, 2018

@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
Copy link

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

Copy link
Member Author

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

Copy link

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.

Copy link
Member Author

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 ☝️ 😕

Copy link

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?

Copy link
Member Author

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 😬

@AllenBW
Copy link
Member Author

AllenBW commented Apr 4, 2018

@kedark3 was gracious enough to make a machine, looks like we're lookin' gooooood!!!

screen shot 2018-04-04 at 11 05 02 am

🌮 💃

@simaishi simaishi removed their assignment Apr 4, 2018
Copy link
Contributor

@himdel himdel left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@himdel himdel merged commit 9eeaab5 into ManageIQ:master Apr 5, 2018
@himdel himdel added this to the Sprint 83 Ending Apr 9, 2018 milestone Apr 5, 2018
@AllenBW AllenBW deleted the BZ/#1557504-ansible-credentails branch April 5, 2018 14:25
simaishi pushed a commit that referenced this pull request Apr 9, 2018
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
@simaishi
Copy link
Contributor

simaishi commented Apr 9, 2018

Gaprindashvili backport details:

$ git log -1
commit 91a990edc68d01ec8d8b1fa50c885091f4b4daba
Author: Martin Hradil <[email protected]>
Date:   Thu Apr 5 16:13:29 2018 +0200

    Merge pull request #1417 from AllenBW/BZ/#1557504-ansible-credentails
    
    Refactors ansible credentials to be more generic, override with dialog input
    (cherry picked from commit 9eeaab534f422a4f5bb2b3517f689dd5597ae127)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1565160

AllenBW added a commit to AllenBW/manageiq-ui-service that referenced this pull request Apr 10, 2018
Also displays any config_info credential object with 'credential' in the key
Fine dup for: ManageIQ#1417
AllenBW added a commit to AllenBW/manageiq-ui-service that referenced this pull request Apr 10, 2018
Also displays any config_info credential object with 'credential' in the key
Fine dup for: ManageIQ#1417
@simaishi
Copy link
Contributor

Backported to Fine via #1418

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants