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-8083] - Linode Create v2 - Disk Encryption #10535

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented May 31, 2024

Description 📝

Preview 📷

Screenshot 2024-05-31 at 1 02 00 PM

How to test 🧪

Prerequisites

  • Make sure you are testing with the Linode Create v2 feature flag on 🏴
  • Use dev environment

Verification steps

  • Verify Disk Encryption shows when you have the the linode_disk_encryption tag on your account and the flag is enabled
  • Verify toggling the checkbox works
  • Verify the checkbox is disabled if the region does not support disk encryption
  • Verify you see a warning for backups when you enable both disk encryption and backups
  • Verify these changes are in parity with what was done in upcoming: [M3-8074] – Add "Disk Encryption" section to Linode Create flow #10462

Known issues

  • Disk encryption stays checked when region is changed
    • This functionality has not been implemented for Linode Create v2, it will come later

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

@bnussman-akamai bnussman-akamai self-assigned this May 31, 2024
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner May 31, 2024 17:06
@bnussman-akamai bnussman-akamai requested review from carrillo-erik and abailly-akamai and removed request for a team May 31, 2024 17:06
Copy link

github-actions bot commented May 31, 2024

Coverage Report:
Base Coverage: 82.34%
Current Coverage: 82.35%

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to test a bit more on Monday, but I noticed these (I believe they stem from the known issue you listed in the description):

  • the Backups notice remains when you change the region (if you previously had "Encrypt Disk" checked)
  • when you change the region from one that supports LDE to one that doesn't, the payload will still have disk_encryption: "enabled"
  • when you change the region from one that supports LDE to one that doesn't, "Encrypted" remains in the summary at the bottom of the page

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good!

Not sure if my comments will be pertinent to this PR, the v1 flow or both, but i noticed a some potential improvements to make to the UI/UX

The tooltip mentions the feature not being available in the selected region, but I have not selected a region yet.
Screenshot 2024-06-03 at 10 20 57

related to the first point:
Screenshot 2024-06-03 at 10 21 27

the way we treat the lack of selected region and unavailability is quite inconsistent with the way it was just done for placement groups. I wonder if it'd be a good time to reel the UX team and confirm the overall messaging. @dwiley-akamai any thoughts?

@bnussman-akamai
Copy link
Member Author

bnussman-akamai commented Jun 3, 2024

I agree with all of that and noticed the same things @abailly-akamai.

I was thinking: Once we get Linode Create v2 "done", we can sync up with UX to establish consistency on those items and implement it in the new create flow.

@dwiley-akamai
Copy link
Contributor

the way we treat the lack of selected region and unavailability is quite inconsistent with the way it was just done for placement groups. I wonder if it'd be a good time to reel the UX team and confirm the overall messaging. @dwiley-akamai any thoughts?

I'd also be in favor of having UX review things comprehensively once the v2 Create flow is done. There are additional inconsistencies across the sections when an unsupported region is selected.

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the issues related to disk encryption staying toggled on when you change the region, things looked good from my additional testing.

Where exactly are we with the error scrolling again? This didn't scroll into view for me:

Screenshot 2024-06-03 at 11 01 22 AM

@bnussman-akamai
Copy link
Member Author

@dwiley-akamai I merged develop in. Scroll into view should work now.

@abailly-akamai
Copy link
Contributor

Sounds good - we should make sure to remember to update both flows when UX changes happen. thanks folks!

@bnussman-akamai
Copy link
Member Author

bnussman-akamai commented Jun 3, 2024

I think the goal would to be only update v2 and launch it with the new UX or after the fact @abailly-akamai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants