Skip to content
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

Conversation

dwiley-akamai
Copy link
Contributor

@dwiley-akamai dwiley-akamai commented Aug 30, 2024

Description πŸ“

Add conditionally-displayed reboot notice on Volume Create page

Target release date πŸ—“οΈ

9/16/24

Preview πŸ“·

Before After
Screenshot 2024-09-10 at 11 29 06β€―AM Screenshot 2024-09-10 at 11 28 14β€―AM

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 πŸ€”

  • πŸ‘€ 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

…ow if user selects a linode that does not support BSE
@dwiley-akamai dwiley-akamai added the Block Storage Encryption (BSE) Relating to Block Storage (Volumes) encryption label Aug 30, 2024
@dwiley-akamai dwiley-akamai self-assigned this Aug 30, 2024
@dwiley-akamai dwiley-akamai marked this pull request as ready for review September 5, 2024 14:26
@dwiley-akamai dwiley-akamai requested review from a team as code owners September 5, 2024 14:26
@dwiley-akamai dwiley-akamai requested review from AzureLatte, cpathipa and hkhalil-akamai and removed request for a team September 5, 2024 14:26
Copy link

github-actions bot commented Sep 5, 2024

Coverage Report: βœ…
Base Coverage: 86.4%
Current Coverage: 86.4%

Copy link
Contributor

@cpathipa cpathipa left a 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.

Copy link
Contributor

@mjac0bs mjac0bs left a 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');
Copy link
Contributor

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?

Copy link
Contributor Author

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

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Sep 10, 2024
@dwiley-akamai dwiley-akamai merged commit 17ad807 into linode:develop Sep 10, 2024
18 of 19 checks passed
@@ -240,6 +243,15 @@ export const VolumeCreate = () => {

const { config_id, linode_id } = values;

const { data: linode } = useLinodeQuery(
linode_id ?? -1,
isBlockStorageEncryptionFeatureEnabled
Copy link
Contributor

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

Suggested change
isBlockStorageEncryptionFeatureEnabled
isBlockStorageEncryptionFeatureEnabled && linode_id !== null

nikhagra-akamai pushed a commit to nikhagra-akamai/manager that referenced this pull request Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Block Storage Encryption (BSE) Relating to Block Storage (Volumes) encryption
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants