-
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: New job improved plan screen #4603
Changes from all commits
1c94fdb
9b7b465
223dae2
2d0805c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import Component from '@ember/component'; | ||
import { or } from '@ember/object/computed'; | ||
|
||
export default Component.extend({ | ||
// Either provide a taskGroup or a failedTGAlloc | ||
taskGroup: null, | ||
failedTGAlloc: null, | ||
|
||
placementFailures: or('taskGroup.placementFailures', 'failedTGAlloc'), | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import Model from 'ember-data/model'; | ||
import attr from 'ember-data/attr'; | ||
import { fragmentArray } from 'ember-data-model-fragments/attributes'; | ||
|
||
export default Model.extend({ | ||
diff: attr(), | ||
failedTGAllocs: fragmentArray('placement-failure', { defaultValue: () => [] }), | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import { get } from '@ember/object'; | ||
import { assign } from '@ember/polyfills'; | ||
import ApplicationSerializer from './application'; | ||
|
||
export default ApplicationSerializer.extend({ | ||
normalize(typeHash, hash) { | ||
hash.FailedTGAllocs = Object.keys(hash.FailedTGAllocs || {}).map(key => { | ||
return assign({ Name: key }, get(hash, `FailedTGAllocs.${key}`) || {}); | ||
}); | ||
return this._super(...arguments); | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import moduleForAcceptance from 'nomad-ui/tests/helpers/module-for-acceptance'; | |
import JobRun from 'nomad-ui/tests/pages/jobs/run'; | ||
|
||
const newJobName = 'new-job'; | ||
const newJobTaskGroupName = 'redis'; | ||
|
||
const jsonJob = overrides => { | ||
return JSON.stringify( | ||
|
@@ -15,15 +16,17 @@ const jsonJob = overrides => { | |
Namespace: 'default', | ||
Datacenters: ['dc1'], | ||
Priority: 50, | ||
TaskGroups: { | ||
redis: { | ||
Tasks: { | ||
redis: { | ||
TaskGroups: [ | ||
{ | ||
Name: newJobTaskGroupName, | ||
Tasks: [ | ||
{ | ||
Name: 'redis', | ||
Driver: 'docker', | ||
}, | ||
}, | ||
], | ||
}, | ||
}, | ||
], | ||
}, | ||
overrides | ||
), | ||
|
@@ -37,7 +40,7 @@ job "${newJobName}" { | |
namespace = "default" | ||
datacenters = ["dc1"] | ||
|
||
task "redis" { | ||
task "${newJobTaskGroupName}" { | ||
driver = "docker" | ||
} | ||
} | ||
|
@@ -313,3 +316,52 @@ test('when submitting a job to a different namespace, the redirect to the job ov | |
); | ||
}); | ||
}); | ||
|
||
test('when the scheduler dry-run has warnings, the warnings are shown to the user', function(assert) { | ||
// Unschedulable is a hint to Mirage to respond with warnings from the plan endpoint | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is helpful! |
||
const spec = jsonJob({ Unschedulable: true }); | ||
|
||
JobRun.visit(); | ||
|
||
andThen(() => { | ||
JobRun.editor.fillIn(spec); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👌 I need to get around to grabbing this editor page object… There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This changes in my next PR too 😢 Since |
||
JobRun.plan(); | ||
}); | ||
|
||
andThen(() => { | ||
assert.ok( | ||
JobRun.dryRunMessage.errored, | ||
'The scheduler dry-run message is in the warning state' | ||
); | ||
assert.notOk( | ||
JobRun.dryRunMessage.succeeded, | ||
'The success message is not shown in addition to the warning message' | ||
); | ||
assert.ok( | ||
JobRun.dryRunMessage.body.includes(newJobTaskGroupName), | ||
'The scheduler dry-run message includes the warning from send back by the API' | ||
); | ||
}); | ||
}); | ||
|
||
test('when the scheduler dry-run has no warnings, a success message is shown to the user', function(assert) { | ||
const spec = hclJob(); | ||
|
||
JobRun.visit(); | ||
|
||
andThen(() => { | ||
JobRun.editor.fillIn(spec); | ||
JobRun.plan(); | ||
}); | ||
|
||
andThen(() => { | ||
assert.ok( | ||
JobRun.dryRunMessage.succeeded, | ||
'The scheduler dry-run message is in the success state' | ||
); | ||
assert.notOk( | ||
JobRun.dryRunMessage.errored, | ||
'The warning message is not shown in addition to the success message' | ||
); | ||
}); | ||
}); |
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.
Did you consider returning the JobPlan record from the adapter
plan
method? Reading this code, I had to go through the model to the adapter to verify that thepeekRecord
would work at this point. Returning a promise with a value instead might make the responsibility of theplan
method clearer.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.
Originally I wrote this as a relationship, then I noped out of there when planning multiple times didn't refresh the relationship on the job side and ended up at this place.
It made sense then to change what was
job.get('jobPlan')
tostore.peekRecord
at the call site, but I appreciate your fresh eyes here. Returning the new record from the plan method is much better.I tested it out and it works, but I already refactored this code in the PR I'm about to open, so instead of making the change here, I'll make it there.