-
Notifications
You must be signed in to change notification settings - Fork 367
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
fix: [M3-7605] - Wrong status indicator when provisioning a LKE #10320
fix: [M3-7605] - Wrong status indicator when provisioning a LKE #10320
Conversation
nodeStatus === 'not_ready' | ||
? 'other' |
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 was the actual fix
import { useInProgressEvents } from 'src/queries/events/events'; | ||
|
||
import NodeActionMenu from './NodeActionMenu'; | ||
import { StyledCopyTooltip, StyledTableRow } from './NodeTable.styles'; |
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 file was basically migrated from NodeTable along with some styling migration
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.
Overall LGTM! left minor comments for code improvements.
- Created a new Kubernetes cluster and confirming the pulsing orange provisioning icon status.
- No visual regressions found.
? 'Provisioning' | ||
: transitionText(instanceStatus ?? '', instanceId ?? -1, recentEvent); | ||
|
||
const displayIP = ip ?? ''; |
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.
Do we need direct usage of ip instead of displayIP
? Since we are conditionally rendering the consumers of displayIP
, which will handle undefined cases, can we directly use ip
?
const nodeReadyAndInstanceRunning = | ||
nodeStatus === 'ready' && instanceStatus === 'running'; | ||
|
||
const iconStatus = | ||
nodeStatus === 'not_ready' | ||
? 'other' | ||
: nodeReadyAndInstanceRunning | ||
? 'active' | ||
: 'inactive'; |
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.
Can we simplify the determination of iconStatus
by directly incorporating the conditions?
const iconStatus =
nodeStatus === 'not_ready'
? 'other'
: nodeStatus === 'ready' && instanceStatus === 'running'
? 'active'
: 'inactive';
Description 📝
Fix the status icon indicator when a Kubernetes Linode is provisioning from grey to pulsing orange. I also went ahead and did some styling/file cleanup.
Preview 📷
Screen.Recording.2024-03-27.at.10.36.48.AM.mov
How to test 🧪
Reproduction steps
(How to reproduce the issue, if applicable)
Verification steps
(How to verify changes)
As an Author I have considered 🤔
Check all that apply