CONSOLE-4075: address tech debt in web hook secret form component #14454
Conversation
|
@Mylanos: This pull request references CONSOLE-4075 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
component/core
|
TODO: Been looking into the ways we use crypto libraries and saw that there is one single instance where we use |
jhadvig
left a comment
jhadvig
left a comment
There was a problem hiding this comment.
One small comment. Otherwise looks good to go 👍
There was a problem hiding this comment.
Lets add a comment that this implementation is a direct replacement for crypto.randomUUID(), and it will generate a cryptographically secure, UUID v4-compliant identifier, for environments where crypto.randomUUID() may not be available.
|
/retest |
|
Following up on our discussions, I have removed the fallback/our own UUID generator function as the |
|
@Mylanos: This pull request references CONSOLE-4075 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
frontend/public/components/secrets/create-secret/WebHookSecretForm.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
nit: if this results in an eslint warning/error I think there should be a // eslint-disable-next-line @typescript-eslint/ban-ts-comment and perhaps a few words explaining why it's needed in the comment
There was a problem hiding this comment.
Seems like that the // eslint-disable-next-line @typescript-eslint/ban-ts-comment won't suppress the generated TS error on its own, so we should probably keep the @ts-ignore / @ts-ignore-next-line.
Will add the comment there, but to leave some answer here as well - briefly the cause is that our current typescript version does not have this function defined yet. But all the browsers that we currently support should have it - https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID#browser_compatibility. But randomUUID should be available starting version 4.6 ( added in this PR ), so once we upgrade our TS this comment can get removed.
logonoff
left a comment
logonoff
left a comment
There was a problem hiding this comment.
works locally, just have some minor nits
|
After the update that incorporated PF components. Screen.Recording.2024-11-08.at.11.42.48.mov |
logonoff
left a comment
logonoff
left a comment
There was a problem hiding this comment.
frontend tests are failing, seems like you need to run yarn i18n and commit
openshift-ci
bot
added
the
kind/i18n
|
/retest |
2 similar comments
|
/retest |
|
/retest |
frontend/public/components/secrets/create-secret/WebHookSecretForm.tsx
Outdated
Show resolved
Hide resolved
|
@Mylanos: This pull request references CONSOLE-4075 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
@logonoff So I discussed with Jon about the extent of the refactors in these components, and he said that the scope of the story was just a logical refactor ( class component to functional ) and the PatternFly updates were out of scope just because these were already big stories. We decided to keep this stories just as a logical refactor and I have created a separate story where we will update all of the components affected in the secrets form tech debt epic. This will make it easier to consolidate the styling between all of the forms and not affect other PRs that are already in review. So I'm holding this PR at this moment I will stash the current changes of my PR (that included the PF components update, that will be used later on in the new story) and roll-back to the commit where I have not used the PF components yet. |
Mylanos
force-pushed
the
CONSOLE-4075-Address-tech-debt-in-WebHookSecretForm-component
branch
from
c3e086e to
8f286d4
Compare
|
/lgtm |
|
Adding labels as this is debt |
openshift-merge-robot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, logonoff, Mylanos 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 |
14 similar comments
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
|
@Mylanos: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Mylanos
deleted the
CONSOLE-4075-Address-tech-debt-in-WebHookSecretForm-component
branch
6 participants
before:
Screen.Recording.2024-11-07.at.16.23.45.mov
after:
Screen.Recording.2024-11-12.at.11.13.26.mov