-
Notifications
You must be signed in to change notification settings - Fork 390
upcoming: [M3-9495] - Disable APL for LKE-E clusters #11809
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
upcoming: [M3-9495] - Disable APL for LKE-E clusters #11809
Conversation
|
Coverage Report: ✅ |
jdamore-linode
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.
Nice, thanks @mjac0bs!
| // Confirm the APL section is not shown. | ||
| cy.findByTestId('apl-label').should('not.exist'); | ||
| cy.findByTestId('apl-radio-button-yes').should('not.exist'); | ||
| cy.findByTestId('apl-radio-button-no').should('not.exist'); | ||
|
|
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.
Related to the above, can we add assertions to confirm that the APL section is present when LKE-E isn't selected?
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 believe this test is already accomplishing that, right? Do you mean toggling to an LKE standard tier and checking that it exists?
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.
Oh yeah, you're absolutely right. There is one distinction I'll call out which is that adding the assertions here gives us explicit coverage that the APL section is present when the LKE-E feature is enabled but not selected during cluster creation, whereas the other test doesn't take the state of LKE-E into account. So if there were ever a logic issue (or mocking issue in the test) involving APL/LKE-E that resulted in APL being hidden, it would get caught by this test but may not get caught by the other.
(I have no idea what the logic for the cluster create page is like, so if this comes across as a farfetched hypothetical we can definitely leave it alone, just wanted to point it out in case it's an important distinction from a dev's POV)
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 left the case of 'LKE-E feature enabled but LKE (standard) cluster tier selected' uncovered in tests because it is essentially the same flow as 'LKE-E feature disabled'. The code sets a default of standard for the selectedTier. The account beta endpoint determines whether or not the APL section is visible to users, and the section visible + the selectedTier determine whether the section is enabled or disabled.
Basically, if there were an issue with the LKE tier selection in the LKE-E flow, the first test could catch it.
abailly-akamai
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.
@mjac0bs What are the UX requirements here?
If the goal is to disable the panel, then that's what we should do, not hide/remove it, especially if this will come at a later time. I find it to be a better UI to disable a panel and add contextual helper text as to the reason why, advertising the feature is coming up, rather than making the user wonder why this is happening and why this isn't possible (or will ever be). This form is already quite complex, and displaying elements conditionally could add confusion IMO.
Thanks, you're right that this is unclear. I'm following up with Product for clarity on this in #dev-apl.
To an extent, we're already doing this (displaying elements conditionally) with the HA form section being hidden once the LKE-E cluster is selected. This was accepted by Product since HA is mentioned as included in the cluster tier section. I see your point that this is a different situation where we'd potentially be "making the user wonder why this is happening and why this isn't possible (or will ever be)". |
|
Reopening this up for review with updated UX treatment, which Product has agreed upon and the APL team is aware of. The description is up to date. |
packages/manager/src/features/Kubernetes/CreateCluster/ApplicationPlatform.tsx
Show resolved
Hide resolved
abailly-akamai
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.
Looks great!
Confirmed new disabled treatment + chip depending on LKE-E ✅
Confirm inputs are properly disabled ✅
|
Found a bug with this, related to the default checked 'No' selection in section disabled state. Going to do some refactoring to address. |
|
Fixed a bug and added a unit test that would catch it. A bad nullish coalescing kept the "No" radio from being checked when the section was enabled, because the value was always
Will merge in the morning after checks pass. |
Cloud Manager UI test results🔺 1 failing test on test run #5 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/linode-config.spec.ts" |
|||||||||||||||||
* Show the APL section only for standard cluster tiers * Added changeset: Disable APL for LKE-E clusters on create flow * WIP - poc for what disabled, rather than hidden, section would look like * Update with proposed UX changes * Update test coverage with mocked endpoints, UX changes * Tweak changeset * Update chip text size to small for consistency with other chips * Fix bug preventing 'no' button from being checked
* Remove optional canvg dependency * Go the rollup external route * Added changeset: Remedy canvg dependency vulnerability * upcoming: [M3-9495] - Disable APL for LKE-E clusters (#11809) * Show the APL section only for standard cluster tiers * Added changeset: Disable APL for LKE-E clusters on create flow * WIP - poc for what disabled, rather than hidden, section would look like * Update with proposed UX changes * Update test coverage with mocked endpoints, UX changes * Tweak changeset * Update chip text size to small for consistency with other chips * Fix bug preventing 'no' button from being checked * test: [M3-9508] - Fix GHA Cypress pipeline by redirecting env value to file (#11853) * Fix GHA Cypress pipeline by using literal values in matrix * Redirect output to .env file * Set `record` action property to `false` (#11854) * Disable Cypress Cloud parallelization action option (#11855) * change: [M3-9434] - Update styles to CDS for create menu (#11821) * change: [M3-9434] - Theme changes to create menu * update styles to CDS * Update CreateMenu.styles.ts * Added changeset: Update styles to CSD for create menu * Update packages/manager/src/features/TopMenu/CreateMenu/CreateMenu.styles.ts Co-authored-by: Hussain Khalil <[email protected]> * Disabling the custom font-weight eslint rule. --------- Co-authored-by: Hussain Khalil <[email protected]> * fix: [M3-9459] - show details button misalignment for selected stackscript (#11838) * fix: [M3-9459] - show details button misalignment for selected stackscript * Added changeset: show details button misalignment for selected stackscript * Remove StyledRootContainer ans use actionCell prop for alignment * Update changeset description * feedback * cleanup * lock update * Switch to using 3.0.1 --------- Co-authored-by: Mariah Jacobs <[email protected]> Co-authored-by: jdamore-linode <[email protected]> Co-authored-by: cpathipa <[email protected]> Co-authored-by: Hussain Khalil <[email protected]> Co-authored-by: hasyed-akamai <[email protected]>
Description 📝
APL won't support LKE-E clusters until later this year (currently roadmapped for Q3). We need to disable it for LKE-E clusters until then and since it's upcoming let users know it is 'coming soon'.
Changes 🔄
isAPLSupportedto checkshowAPLand theselectedTierlke-create.spec.tsto check that an LKE-E cluster tier selection results in the section being disabledApplicationPlatform.test.tsx@hasyed-akamai's PR #11581 will need to replicate these changes for parity in the RHF cluster create flow.
Target release date 🗓️
3/25
Preview 📷
In summary, this was the change:
A more comprehensive reference of all the other possible states:
How to test 🧪
Prerequisites
(How to setup test environment)
Reproduction steps
(How to reproduce the issue, if applicable)
Verification steps
(How to verify changes)
Author 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 ✅