-
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: Remodel task-group-deployment-summary to properly use PlacedCanaries #4325
Conversation
6a2e707
to
1f83916
Compare
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.
Cool some of this is a bit project specific over my head-ness, but just out of interest, where these tests passing previously?
|
||
export default ApplicationSerializer.extend({ | ||
normalize(typeHash, hash) { | ||
hash.PlacedCanaryAllocations = hash.PlacedCanaries || []; |
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 you need this if you are setting the defaultValue above? (or vice versa)
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.
defaultValue
isn't invoked when the value is null
(as opposed to undefined
), so this is needed. I suppose the defaultValue
in the model isn't strictly necessary, but I like that it kinda doubles as a type annotation.
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.
👍
@@ -5,7 +5,8 @@ import { test } from 'qunit'; | |||
import moment from 'moment'; | |||
import moduleForAcceptance from 'nomad-ui/tests/helpers/module-for-acceptance'; | |||
|
|||
const sum = (list, key) => list.reduce((sum, item) => sum + get(item, key), 0); | |||
const sum = (list, key, getter = a => a) => | |||
list.reduce((sum, item) => sum + getter(get(item, key)), 0); |
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.
Nice helper!
@johncowen The tests were passing previously, but that's because the fixture data was also modeled incorrectly. So the process went:
|
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. |
PlacedCanaries is a
[]string
of allocation IDs, not anint
.The result of the UI getting this wrong was whenever the count of placed canaries was shown, it was 0. Now it uses the length of PlacedCanaries.