-
Notifications
You must be signed in to change notification settings - Fork 361
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-8420] β Add conditionally-displayed reboot notice on Volume Create page #10868
upcoming: [M3-8420] β Add conditionally-displayed reboot notice on Volume Create page #10868
Conversation
β¦ow if user selects a linode that does not support BSE
β¦d as well, and disable Create button when that notice is being displayed
β¦t notice to Volume Create page
Coverage Report: β
|
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.
LGTM! Confirming on the functionality and client libraries notice in Volume create flow.
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.
Confirmed the reboot notice showed and create button disabled once the checkbox was checked for a linode without the bse capability, and that the spacing and alignment looks good. With the feature flag off, no notice or disabled button, as expected.
@@ -132,6 +132,10 @@ describe('volume create flow', () => { | |||
.should('be.visible') | |||
.click(); | |||
|
|||
// @TODO BSE: once BSE is fully rolled out, check for the notice (selected linode doesn't have | |||
// blockstorage_encryption capability + user checked "Encrypt Volume" checkbox) instead of the absence of it | |||
cy.findByText(CLIENT_LIBRARY_UPDATE_COPY).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.
When manually testing, rebooting a linode without the bse capability didn't result in an upgraded qemu version/bse capability for me - not sure if that's expected in dev. I might have missed it, but wasn't seeing anywhere in tests where we were confirming that a user with a bse capability on the linode would no longer see the checkbox. Did we want to confirm this?
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.
Raised the reboot --> upgrade question in the project channel, not sure if that is fully in place yet.
Added test coverage for the case where the linode supports BSE
@@ -240,6 +243,15 @@ export const VolumeCreate = () => { | |||
|
|||
const { config_id, linode_id } = values; | |||
|
|||
const { data: linode } = useLinodeQuery( | |||
linode_id ?? -1, | |||
isBlockStorageEncryptionFeatureEnabled |
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.
Late to this PR, but noticed a minor issue that can be resolved in a later change:
To prevent an API call for linodes/instances/-1
isBlockStorageEncryptionFeatureEnabled | |
isBlockStorageEncryptionFeatureEnabled && linode_id !== null |
β¦lume Create page (linode#10868)
Description π
Add conditionally-displayed reboot notice on Volume Create page
Target release date ποΈ
9/16/24
Preview π·
How to test π§ͺ
Prerequisites
Point at the dev environment with the
blockstorage-encryption
tag on your account.Verification steps
Pointing at the dev environment:
With the BSE feature flag off, you should not see a client library update reboot notice regardless of which linode you select.
With the BSE feature flag on, if you select a linode that does not have the
blockstorage_encryption
capability & check the "Encrypt Volume" checkbox, you should see the client library update reboot notice below the Linode dropdown. The "Create Volume" button should also be disabled.Confirm the alignment of the Linode & Config dropdowns look good in both cases, and across viewport widths (they stack in mobile view)
As an Author I have considered π€