-
Notifications
You must be signed in to change notification settings - Fork 358
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-8032] β Add Encrypted/Not Encrypted status to Node Pool table #10480
upcoming: [M3-8032] β Add Encrypted/Not Encrypted status to Node Pool table #10480
Conversation
β¦w, temp placeholders pending upcoming work
β¦'Encrypted' in Linode Create summary panel, remove temp code in several components
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.
With mocks on, I:
- Verified that the node pools with
disk_encryption
enabled/disabled status displays the expected UI text and icons. - Verified that encryption status isn't visible if the feature flag is off.
What's the plan for test coverage of LDE changes?
packages/manager/src/features/Kubernetes/KubernetesClusterDetail/NodePoolsDisplay/NodePool.tsx
Show resolved
Hide resolved
packages/manager/src/features/Kubernetes/KubernetesClusterDetail/NodePoolsDisplay/NodeTable.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Kubernetes/KubernetesClusterDetail/NodePoolsDisplay/NodeTable.tsx
Outdated
Show resolved
Hide resolved
The earlier tickets in the epic had mentions of tests but looks like some of the later ones are missing it. I'll go back and update those tickets. In the meantime, I added unit test coverage in the most recent commit |
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.
Confirming display and styling, looks good!
Left comments for code cleanup which should be a very quick fix
packages/manager/src/features/Kubernetes/KubernetesClusterDetail/NodePoolsDisplay/NodeTable.tsx
Outdated
Show resolved
Hide resolved
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.
Thanks for addressing feedback. I'm approving pending:
- Alban's feedback is addressed
- Tests pass - on this last run, the second runner didn't run at all, probably due to some Jenkins issue because prior to last commit, CI was passing
packages/manager/src/features/Kubernetes/KubernetesClusterDetail/NodePoolsDisplay/NodeTable.tsx
Outdated
Show resolved
Hide resolved
β¦name; update NodeTable.test.tsx
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.
Thanks for addressing the changes! approving granted all tests pass π’
Description π
Add encryption status indication to the Node Pool table.
Changes π
src/assets/icons
disk_encryption
inNodePoolsDisplay.tsx
, pass it down through toNodeTable.tsx
NodePool.tsx
Target release date ποΈ
5/28/24
Preview π·
How to test π§ͺ
The easiest way to test would be to turn on the mocks and navigate to an LKE Details page, since the mocks I added cover the encrypted and unencrypted cases. Otherwise, you'd need your account in alpha to have a "legacy" cluster & pool to see the unencrypted variant.
Verification steps
pools?page_size=200
disk_encryption
field for each pool. It should correspond with what's shown in the UI for each.As an Author I have considered π€