Enhance OpenStack provider help text fields #861
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import React, { useCallback, useReducer } from 'react'; | ||
| import { Trans } from 'react-i18next'; | ||
| import { Base64 } from 'js-base64'; | ||
| import { | ||
| openstackSecretFieldValidator, | ||
|
|
@@ -7,7 +8,16 @@ import { | |
| } from 'src/modules/Providers/utils'; | ||
| import { useForkliftTranslation } from 'src/utils/i18n'; | ||
|
|
||
| import { | ||
| Divider, | ||
| FileUpload, | ||
| Form, | ||
| FormGroup, | ||
| Popover, | ||
| Radio, | ||
| Switch, | ||
| } from '@patternfly/react-core'; | ||
| import HelpIcon from '@patternfly/react-icons/dist/esm/icons/help-icon'; | ||
|
|
||
| import { EditComponentProps } from '../BaseCredentialsSection'; | ||
|
|
||
|
|
@@ -22,6 +32,29 @@ import { | |
| export const OpenstackCredentialsEdit: React.FC<EditComponentProps> = ({ secret, onChange }) => { | ||
| const { t } = useForkliftTranslation(); | ||
|
|
||
| const insecureSkipVerifyHelperTextMsgs = { | ||
| error: t('Error: this field must be set to a boolean value.'), | ||
| successAndSkipped: t("The provider's CA certificate won't be validated."), | ||
| successAndNotSkipped: t("The provider's CA certificate will be validated."), | ||
| }; | ||
|
|
||
| const cacertHelperTextMsgs = { | ||
| error: t( | ||
| 'Error: The format of the provided CA Certificate is invalid. Ensure the CA certificate format is valid.', | ||
|
There was a problem hiding this comment. First ooccurance: please change to "CA certificate" |
||
| ), | ||
| success: t( | ||
| 'A CA certificate to be trusted when connecting to the OpenStack Identity (Keystone) endpoint. Ensure the CA certificate format is valid. To use a CA certificate, drag the file to the text box or browse for it. To use the system CA certificates, leave the field empty.', | ||
| ), | ||
|
There was a problem hiding this comment. Is system CA certificates supposed to be plural? [I don't know] There was a problem hiding this comment. both works for me, it's one certificate per our provider, but the system may have more then one for other servers, so both singular and plural are ok with me. There was a problem hiding this comment. I'll change to a singular phrasing since the current text refers to the ca assigned to our provider: |
||
| }; | ||
|
|
||
| const insecureSkipVerifyHelperTextPopover = ( | ||
| <Trans t={t} ns="plugin__forklift-console-plugin"> | ||
| Note: If {"'"}Skip certificate validation{"'"} is selected, migrations from this provider will | ||
| not be secure.{'<br><br>'}Insecure migration means that the transferred data is sent over an | ||
| insecure connection and potentially sensitive data could be exposed. | ||
|
There was a problem hiding this comment. "not be secure[tags], meaning that the transferred data..." There was a problem hiding this comment.
|
||
| </Trans> | ||
| ); | ||
|
|
||
| const authType = safeBase64Decode(secret?.data?.authType || ''); | ||
| const username = safeBase64Decode(secret?.data?.username || ''); | ||
| const insecureSkipVerify = safeBase64Decode(secret?.data?.insecureSkipVerify || '') === 'true'; | ||
|
|
@@ -62,6 +95,7 @@ export const OpenstackCredentialsEdit: React.FC<EditComponentProps> = ({ secret, | |
| authenticationType: authenticationType, | ||
| validation: { | ||
| cacert: 'default' as Validation, | ||
| insecureSkipVerify: 'default' as Validation, | ||
| }, | ||
| }; | ||
|
|
||
|
|
@@ -151,7 +185,9 @@ export const OpenstackCredentialsEdit: React.FC<EditComponentProps> = ({ secret, | |
| role="radiogroup" | ||
| fieldId="authType" | ||
| label={t('Authentication type')} | ||
| helperText={t( | ||
| 'Method of authentication to use when connecting to the OpenStack Identity (Keystone) server.', | ||
| )} | ||
| > | ||
| <Radio | ||
| name="authType" | ||
|
|
@@ -212,6 +248,21 @@ export const OpenstackCredentialsEdit: React.FC<EditComponentProps> = ({ secret, | |
|
|
||
| <FormGroup | ||
| label={t('Skip certificate validation')} | ||
| labelIcon={ | ||
| <Popover | ||
| headerContent={<div>Skip certificate validation</div>} | ||
| bodyContent={<div>{insecureSkipVerifyHelperTextPopover}</div>} | ||
| alertSeverityVariant="info" | ||
| > | ||
| <button | ||
| type="button" | ||
| onClick={(e) => e.preventDefault()} | ||
| className="pf-c-form__group-label-help" | ||
| > | ||
| <HelpIcon noVerticalAlign /> | ||
| </button> | ||
| </Popover> | ||
| } | ||
| fieldId="insecureSkipVerify" | ||
| validated={state.validation.insecureSkipVerify} | ||
| helperTextInvalid={t('Error: Insecure Skip Verify must be a boolean value.')} | ||
|
|
@@ -220,9 +271,9 @@ export const OpenstackCredentialsEdit: React.FC<EditComponentProps> = ({ secret, | |
| className="forklift-section-secret-edit-switch" | ||
| id="insecureSkipVerify" | ||
| name="insecureSkipVerify" | ||
| label={insecureSkipVerifyHelperTextMsgs.successAndSkipped} | ||
| labelOff={insecureSkipVerifyHelperTextMsgs.successAndNotSkipped} | ||
| aria-label={insecureSkipVerifyHelperTextMsgs.successAndSkipped} | ||
| isChecked={insecureSkipVerify} | ||
| hasCheckIcon | ||
| onChange={(value) => handleChange('insecureSkipVerify', value ? 'true' : 'false')} | ||
|
|
@@ -231,15 +282,13 @@ export const OpenstackCredentialsEdit: React.FC<EditComponentProps> = ({ secret, | |
| <FormGroup | ||
| label={ | ||
| insecureSkipVerify | ||
| ? t("CA certificate - disabled when 'Skip certificate validation' is checked") | ||
|
There was a problem hiding this comment. @RichardHoch This is a Switch component for selecting the options, so I wonder which action is commonly used for a Switch clicking. I checked other documentations and saw that they use |
||
| : t('CA certificate - leave empty to use system CA certificates') | ||
| } | ||
|
There was a problem hiding this comment. here I'll leave the plural phrasing... |
||
| fieldId="cacert" | ||
| helperText={cacertHelperTextMsgs.success} | ||
| validated={state.validation.cacert} | ||
| helperTextInvalid={cacertHelperTextMsgs.error} | ||
| > | ||
| <FileUpload | ||
| id="cacert" | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
urlValidationHook is a validator, it should be implemented in the validations part of our project, please move it to currect directory.
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.
This was not added as part of this PR, I just added more logic to that method.
In addition, this validation method exists for all providers as part of the modal code (ocp, oVirt,vsphere), so better do that in a separate follow up 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.
"you touch it you own it", it was not in the right place before you added to it, and it's not in it's right place now. Since you added to it, can you move it to the currect place ?
p.s.
you will also need to rename to fit the validation methods naming convention
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'm ok with a folow up PR to fix in all places this method is used out of place
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.
#864