Skip to content

Conversation

@jrafanie
Copy link
Member

@jrafanie jrafanie commented May 1, 2024

This PR simplifies a few descriptions from #16

Updated screens:

image

@miq-bot
Copy link
Member

miq-bot commented May 1, 2024

Checked commits jrafanie/manageiq-providers-embedded_terraform@0894742~...3562377 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. 🍰

:component => 'password-field',
:label => N_('IBM Cloud API Key'),
:helperText => N_('The API key for IBM Cloud. A value for this field is required if classic user name and classic API key are not provided. A valid connection must have value for IBM Cloud API Key or, IBM Cloud Classic Infrastructure User Name and IBM Cloud Classic Infrastructure API Key.'),
:helperText => N_('The API key for IBM Cloud. This credential requires either an IBM Cloud API Key or both a Classic Infrastructure User Name and Classic Infrastructure API key.'),
Copy link
Member

Choose a reason for hiding this comment

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

Does something like this work?

Suggested change
:helperText => N_('The API key for IBM Cloud. This credential requires either an IBM Cloud API Key or both a Classic Infrastructure User Name and Classic Infrastructure API key.'),
:helperText => N_('The API key for IBM Cloud. This credential requires either an IBM Cloud API Key or a Classic Infrastructure User Name / API key pair.'),

Copy link
Member

Choose a reason for hiding this comment

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

Or perhaps

Does something like this work?

Suggested change
:helperText => N_('The API key for IBM Cloud. This credential requires either an IBM Cloud API Key or both a Classic Infrastructure User Name and Classic Infrastructure API key.'),
:helperText => N_('The API key for IBM Cloud. This credential requires either an IBM Cloud API Key or a Classic Infrastructure User Name and API key.'),

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, English vs. logical makes that not obvious. I think I like the second one. I'm not a fan of duplicating the same thing 3 times here so do wish we could put this logic into the validation for this form though so we can help the user if they don't provide the right things.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed - maybe some tabs? like we do for multiple auths?

Copy link
Member

Choose a reason for hiding this comment

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

actually, separate credential types feels easier

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, let me split off this into a new PR and get the other change in. I'll create a new credential type and play with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, split it off into a separate class. This will allow us to test both IBM Cloud and IBM Classic Infrastructure: #23

@jrafanie jrafanie force-pushed the simplify_descriptions branch from 3562377 to 0894742 Compare May 2, 2024 22:04
@jrafanie jrafanie changed the title Simplify descriptions from credentials PR #16 Simplify description from credentials PR #16 May 2, 2024
@jrafanie
Copy link
Member Author

jrafanie commented May 2, 2024

NOTE: I'll split off the IBM Cloud credentials into a followup PR with two types: cloud and classic infrastructure.

Note, for the IBM cloud, it looks like we need either the cloud API key or both the classic infra user name and classic infra API key so it doesn't make sense to require the cloud API key. Is this correct @putmanoj?

image

@jrafanie
Copy link
Member Author

This should be ready to go

@agrare agrare merged commit 327959b into ManageIQ:master May 20, 2024
@jrafanie jrafanie deleted the simplify_descriptions branch May 22, 2024 13:28
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.

5 participants