-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
0308cd0
to
2d0805c
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.
Looks good to me! 👍
I had one thought about the plumbing of the JobPlan model, comment inline.
const planOutput = yield this.get('model').plan(); | ||
this.set('planOutput', planOutput.Diff); | ||
yield this.get('model').plan(); | ||
const plan = this.get('store').peekRecord('job-plan', this.get('model.id')); |
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 the peekRecord
would work at this point. Returning a promise with a value instead might make the responsibility of the plan
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')
to store.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.
JobRun.visit(); | ||
|
||
andThen(() => { | ||
JobRun.editor.fillIn(spec); |
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 need to get around to grabbing this editor page object…
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 changes in my next PR too 😢
Since registerHelper
only applies to acceptance tests, I have to do some Not Cool™ things to make this work in a component integration test.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is helpful!
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. |
Part of the Job Writes project: #4600
When planning a job, nomad provides a "scheduler dry-run" which can be used to catch issues before the job is actually ran. Now this same output will be shown in the UI before running a job.