-
Notifications
You must be signed in to change notification settings - Fork 2k
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
UI: Node drain and eligibility #4353
Conversation
This makes it match the CLI node status output
This makes it easier to scan for interesting nodes
The cloest Moment.js has is "homanize" which isn't precise enough.
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 is great! 👍 Just one thought about clarity in the format-duration util.
} else if (units === 'mms') { | ||
durationParts.microseconds = duration % 1000; | ||
duration = Math.floor(duration / 1000); | ||
} |
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.
I found this part a little tricky to follow. My reading is that the unit of duration
can be one of nanoseconds, microseconds, or milliseconds, and for the former two cases we're normalizing it back to milliseconds here.
If that's correct, it might also be clearer to mutate the units
argument in these blocks also, rather than in the dense ternary on the following line. And perhaps some commentary explaining this might also help?
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.
Units can be any unit moment supports (ms, s, m, h, days, months, years) as well as microseconds and nanoseconds, which moment doesn't support.
So this ternary:
const momentDuration = moment.duration(duration, ['ns', 'mms'].includes(units) ? 'ms' : units);
Uses units as is unless units is either microseconds or nanoseconds, in which case nanos and micros need to be calculated upfront and duration needs to be normalized as milliseconds.
But your suggestion to reassign units
to ms
is still valid. Also point taken about needing comments.
@@ -75,6 +76,18 @@ test('/clients/:id should list additional detail for the node below the title', | |||
.includes(node.httpAddr), | |||
'Address is in additional details' | |||
); | |||
assert.ok( |
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.
Not that you need another testing framework, but qunit-dom
has been nice to use for tests like this (and you don't need to do all of the trim / includes stuff).
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.
My testing backlog includes
- qunit-dom
- ember-cli-page-object
- testing unification
- async/await
- refactoring how I use mirage now that I know it better
Some day!
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Changes to clients list:
Changes to client detail:
Clients list with a client that is draining
Client detail for a client that is draining with a deadline
Client detail for client that is force-draining (rare to see given the nature of a forced drain)