-
Notifications
You must be signed in to change notification settings - Fork 389
refactor: [M3-8777] - Refactor CreateCluster component to use react-hook-form #11581
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
refactor: [M3-8777] - Refactor CreateCluster component to use react-hook-form #11581
Conversation
…to-use-react-hook-form
packages/manager/src/features/Kubernetes/CreateCluster/ControlPlaneACLPane.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Kubernetes/CreateCluster/CreateCluster.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Kubernetes/CreateCluster/ControlPlaneACLPane.tsx
Outdated
Show resolved
Hide resolved
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.
Hey @hasyed-akamai, since we already talked about the merge conflicts to resolve to keep parity with recent ACL changes (must be enabled in LKE-E), I'm going to mention a couple of other issues I noticed while testing the branch in its current state:
- When the LKE-E tier is selected, the Version field doesn't autopopulate, and this also results in no LKE version if the user switches back to the standard tier.
| Develop | This Branch |
|---|---|
develop-version-prefill.mov |
version-prefill-regression-this-branch.mov |
- For a standard LKE tier, when the user selects a Region and Plan type and then selects HA/No HA, the Create Cluster button stays disabled and no cluster can be created. Interestingly, the button seems to enable itself if the user toggles the ACL switch after that.
| Prod | This Branch |
|---|---|
ha-selection-prod.mov |
regression-ha-this-branch.mov |
One thing I did not test at all yet, because I have to figure out how to enable it, is App Platform for Kubernetes (APL). (noting this as a reminder to myself, as well)
…to-use-react-hook-form
mjac0bs
left a comment
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.
Appreciate you doing this refactor, @hasyed-akamai!
With LKE-E being so close to launching in LA, I want to be absolutely sure that there aren't any regressions when we switch over to RHF. Functionality was looking good from my testing so far - thanks for addressing the initial feedback; see the APL PR for one more thing.
I'm still reading through the code changes and will continue that on Monday. Publishing some current comments so you can address them as I go.
packages/manager/src/features/Kubernetes/CreateCluster/CreateCluster.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Kubernetes/CreateCluster/ClusterTierPanel.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Kubernetes/CreateCluster/ControlPlaneACLPane.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Kubernetes/CreateCluster/ControlPlaneACLPane.tsx
Show resolved
Hide resolved
packages/manager/src/features/Kubernetes/CreateCluster/ControlPlaneACLPane.tsx
Show resolved
Hide resolved
packages/manager/src/features/Kubernetes/CreateCluster/CreateCluster.tsx
Outdated
Show resolved
Hide resolved
|
Waiting on LKE-E release |
|
Noting this for future consideration: |
|
This PR is stale because it has been open 15 days with no activity. Please attend to this PR or it will be closed in 5 days |
Cloud Manager UI test results🔺 3 failing tests on test run #39 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/linode-config.spec.ts,cypress/e2e/core/linodes/search-linodes.spec.ts,cypress/e2e/core/linodes/resize-linode.spec.ts" |
|||||||||||||||||||||||
…to-use-react-hook-form
|
🫠 Oh no. This is messy because of the eslint overhaul. Merging develop in added a bunch of linting changes in other files. Reverting the merge commit made it worse? I think this PR needs to be closed and the RHF changes need to be cherry picked onto a branch that is up to date with develop. @hasyed-akamai - I'm going to try this and see if that works better. In 955ed0a I had the merge conflicts resolved and things seemed to be working well, except the IP ACL error, which needed its validation handling updated to play nicely with RHF. An integration test failed as an indicator of that. Also considering a feature flag to render the RHF flow and fall back on the old flow in case there's a major bug in prod when this releases. 🤔 |
Description 📝
This PR refactors the CreateCluster component to use
react-hook-formfor form state management. It also removes theMultipleIPInputcomponents for cleaner code.Changes 🔄
Target release date 🗓️
N/A
Verification steps
developwith and without the LKE-Enterprise LA feature flag onAuthor Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅