-
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-8019] β Add Encrypted/Not Encrypted status to Linode Detail summary header #10537
upcoming: [M3-8019] β Add Encrypted/Not Encrypted status to Linode Detail summary header #10537
Conversation
β¦Linode Detail header; adjust several mocks and test files based on update to Linode type
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.
- Confirmed that with the LDE flag in our dev tool toggled off, I do not see any LDE-related things in the UI.
- Confirmed the displayed encryption status matches what the disk_encryption field in the linode detail network request indicates (enabled --> Encrypted, disabled --> Not Encrypted).
- Confirmed the tooltip matches the screenshots in the Preview section for standard vs LKE linodes.
- Confirmed the encrypted status and tooltip look good upon screen resizing.
- Confirmed no styling regressions to the encryption status on the LKE details page.
Thanks for the test coverage too! Left a few minor comments.
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.
Should this be an Added
changeset?
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.
From my perspective, for the api-v4
package, "Added" indicates a newly-created type or interface, whereas "Changed" would include existing types and interfaces being expanded with new properties. Open to changing this based on additional thoughts, but it's probably a good Cafe item so a convention can be set.
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.
Your perspective makes sense - I am curious to hear what other members of the team think fits best here. Totally possible that I'm just thinking about it differently.
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.
(In support of leaving this changeset as is, I can add a comment to our next changelog doc when the team reviews pre-release just to clarify our convention, and we won't need to dedicate any cafe time.)
@@ -22,3 +22,9 @@ export const DISK_ENCRYPTION_BACKUPS_CAVEAT_COPY = | |||
|
|||
export const DISK_ENCRYPTION_NODE_POOL_GUIDANCE_COPY = | |||
'To enable disk encryption, delete the node pool and create a new node pool. New node pools are always encrypted.'; | |||
|
|||
export const UNENCRYPTED_STANDARD_LINODE_GUIDANCE_COPY = | |||
'Use Rebuild to enable or disable disk encryption.'; |
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.
This copy feels a little strange when it begins with the two verbs next to each other. Would something like Rebuild your Linode to enable or disable disk encryption.
be better here?
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.
Will bring this to UX for consideration π
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.
Revised to Rebuild this Linode to enable or disable disk encryption.
after revisiting the copy with UX π
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.
Verified detail header correctly reflects disk_encryption
value and layout scales with the window as expected.
Approved pending @mjac0bs' comments regarding changelog and copy wording.
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.
Approving, pending we follow up on possible copy update (pending UX feedback) in this ticket or a future one. π
Description π
Add Encrypted/Not Encrypted status to Linode Detail summary header.
Changes π
lke_cluster_id
to Linode interface, update mocks and several test files accordinglyEncrypted
andNot Encrypted
status to Linode Detail summary headerTarget release date ποΈ
6/10
Preview π·
Screenshots
Encrypted:
Unencrypted & is LKE Linode:
Unencrypted & is standard Linode:
How to test π§ͺ
To test each of the three cases outlined by the screenshots, it'll be easiest to use the MSW and make adjustments to these lines:
manager/packages/manager/src/mocks/serverHandlers.ts
Lines 768 to 778 in 0e2d367
Prerequisites
Verification steps
Without the tag and/or with the LDE flag in our dev tool toggled off, you should not see any LDE-related things in the UI. Otherwise,
disk_encryption
field in the linode detail network request indicates (enabled
-->Encrypted
,disabled
-->Not Encrypted
)As an Author I have considered π€