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
wizard page going blank for KMS #316
Conversation
SanjalKatiyar
commented
Jun 21, 2022
@@ -86,11 +87,13 @@ export const VaultConfigure: React.FC<KMSConfigureProps> = ({ | |||
|
|||
const setAuthMethod = React.useCallback( | |||
(authMethod: VaultAuthMethods) => { | |||
!!vaultStateClone.authMethod && setAuthValue(''); |
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.
Are we sure this is the causing issue?
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.
no it is extra fix...
when we change the authmethod we want to reset the authvalue...
so basically go back to initial state...
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.
encryption
in ValutConnectionForm
is causing the issue
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.
no it is extra...
when we change the authmethod we want to reset the authvalue...
so basically go back to initial state...
earlier we were just resetting the value to ''
but we want to reset valid as true
as well (initial state)...
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.
true? no authValue is string only, the default value is ''
authValue: {
value: '',
valid: true,
},
you are doing the same.
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.
yeah... when we reset we want to go back to initial state which was, right ??
That is what I am doing...
authValue: {
value: '',
valid: true,
},
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.
no authValue is string only, the default value is --> yes... but valid
is true by default...
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.
not required... we are re-setting the state right... so we don't need validation...
we need validation when we type/input the authvalue... we have setAuthValue function for that...
here we are explicitly setting it to ''
to if we add validation it will store authValue.valid as false
(which is not initial state)
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.
the issue is when you change auth method, the auth value becomes empty but it still allows you to create the KMS config. Please check.
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.
mmm no, it will work
/re-test |
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GowthamShanmugam, SanjalKatiyar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherry-pick release-4.11 |
@SanjalKatiyar: once the present PR merges, I will cherry-pick it on top of release-4.11 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-4.11-compatibility |
@SanjalKatiyar: once the present PR merges, I will cherry-pick it on top of release-4.11-compatibility in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@SanjalKatiyar: new pull request created: #328 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@SanjalKatiyar: new pull request created: #329 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |