-
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] Adds meta to job list stub and displays a pack logo on the jobs index #14833
Conversation
Ember Asset Size actionAs of 2ba343c Files that got Bigger 🚨:
Files that got Smaller 🎉:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
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.
Good job buddy.
ui/app/routes/jobs/index.js
Outdated
@@ -32,7 +32,7 @@ export default class IndexRoute extends Route.extend( | |||
controller.set('namespacesWatch', this.watchNamespaces.perform()); | |||
controller.set( | |||
'modelWatch', | |||
this.watchJobs.perform({ namespace: controller.qpNamesapce }) | |||
this.watchJobs.perform({ namespace: controller.qpNamesapce, meta: true }) |
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.
unrelated
nit
: Do you see that typo to the left of your change?
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.
unrelated
question
: We don't poll /jobs
though? I was thinking about removing this. Any concerns you can think of while we're here?
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.
We do poll existing jobs for changes; if you are on the index page and a pending
job changes to ready
, for example, that update will propagate to the list.
@@ -35,7 +35,7 @@ export default class JobRoute extends Route { | |||
const relatedModelsQueries = [ | |||
job.get('allocations'), | |||
job.get('evaluations'), | |||
this.store.query('job', { namespace }), | |||
this.store.query('job', { namespace, meta: true }), |
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.
question
non-blocking
: We're making a second request to /jobs/:jobId
here. Maybe we should refactor line 33 .findRecord
to .query
instead?
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.
LGTM! Let's get that field added to api.JobListStub
and ship!
nomad/structs/structs.go
Outdated
@@ -4720,6 +4734,7 @@ type JobListStub struct { | |||
ModifyIndex uint64 | |||
JobModifyIndex uint64 | |||
SubmitTime int64 | |||
Meta map[string]string |
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.
We should make sure this field is included on api.JobListStub
as well.
We should probably add a api.JobListWithFields
method to the api
package as well (under api/jobs.go), but I'm fine with shipping this PR without and we can come back to that later: right now the UI will be the only consumer of this field.
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 one!
In addition to the update to the api
package we need to update the docs as well:
https://github.com/hashicorp/nomad/blob/main/website/content/api-docs/jobs.mdx?plain=1#L27-L47
My comment about the omitempty
can be ignored, I'm not sure if it's a good idea or not give how it behaves kind of weird.
38f3a94
to
a1d646a
Compare
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on main:
|
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.
LGTM @philrenaud! I've left the one fix we'll need for capitalization of the field in the api
package, but other than that it's ready to ship!
Co-authored-by: Tim Gross <tgross@hashicorp.com>
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. |
Resolves #11886
Adds a Pack logo to the jobs list index's job row: