-
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
Basic services table implemented #13856
Basic services table implemented #13856
Conversation
Ember Asset Size actionAs of 24fd942 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
7310077
to
ff79a48
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.
Hey, buddy strong start! Some blocking feedback from me:
- Can we have this PR merge into a trunk branch instead of main?
- Can we handle the creation of the
Service
model andServiceFragment
refactoring into its own PR? I support atomic PRs, but in this case, I'd suggest breaking this PR down as follows:
ServiceFragment
model refactoring with regression testing. (isolate refactoring)Service
model creation to what we have in the backend. And associatedAdapter
andSerializer
logic. (isolate core logic)- Handle basic
ServicesTable
implementation. (view layer implementation)
This way you can make sure that we're not adding namespaces
later. We need to implement them along the entire process.
@@ -1,11 +1,10 @@ | |||
import { attr } from '@ember-data/model'; | |||
import Fragment from 'ember-data-model-fragments/fragment'; | |||
import { fragment } from 'ember-data-model-fragments/attributes'; | |||
// import { fragment } from 'ember-data-model-fragments/attributes'; |
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.
nit
: Unused import.
ui/app/models/service.js
Outdated
@attr('string') name; | ||
@attr('string') portLabel; | ||
export default class Service extends Model { | ||
@attr('string') ServiceName; |
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.
nit
: Why is this capitalized?
export default class Service extends Fragment { | ||
@attr('string') name; | ||
@attr('string') portLabel; | ||
export default class Service extends Model { |
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.
blocking
: When a Service
is registered it must be associated with a nodeId
, allocId
and job
. However, I don't see those being modeled 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.
Answered this elsewhere, but these are not properties that return in the list view for Services. As I build out pieces of the UI that use things like nodeID, alloId, and job, I'll model those as well.
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.
Generally, I'm not a fan of modeling as you go. And I think not modeling Variables
specifically for building the URL ended up making that work downstream much more difficult. I'm already noticing a decent number of lurking bugs that can arise later if we don't take the time to model correctly at first.
return RSVP.hash({ | ||
services: this.store.findAll('service'), | ||
}).catch(notifyForbidden(this)); |
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.
blocking
: We're only resolving one promise here and setting the model. I don't think we need RSVP
here.
@@ -35,7 +35,7 @@ export default class TaskGroup extends Fragment { | |||
|
|||
@fragmentArray('task') tasks; | |||
|
|||
@fragmentArray('service') services; | |||
@fragmentArray('service-fragment') services; |
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
: The TaskGroup
and TaskDetail
views are two of the highest churn files in the codebase. Did you test the existing set-up with all the different services
combinations to ensure we're not introducing a regression here?
this.get('/services', function (schema) { | ||
return [ | ||
{ | ||
Namespace: 'default', |
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.
blocking:
Be careful hard coding namespaces even if its just your mirage configuration. Ripping this out later will be harder than starting with implementing namespaces.
import { pickOne } from '../utils'; | ||
import { provide } from '../utils'; |
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.
nit
: Duplicate import path.
ServiceName: () => `${faker.hacker.adjective()}-${faker.hacker.noun()}`, | ||
Tags: () => { |
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
: Why aren't we adapting the properties to have lowercase property keys in the Adapter layer?
test('services table shows expected number of services', async function (assert) { | ||
server.createList('service', 3); | ||
await Services.visit(); | ||
assert.equal( | ||
findAll('[data-test-service-row]').length, | ||
3, | ||
'correctly shows 3 services' | ||
); |
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.
blocking
: Services are namespaced. Per the RFC. There should be a test here that ensures that we're handling that here.
export default class Service extends Fragment { | ||
@attr('string') name; | ||
@attr('string') portLabel; | ||
export default class Service extends Model { |
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.
blocking
: The schema seems like its missing some key relationships and properties per what we have on the backend.
https://github.com/hashicorp/nomad/blob/main/api/services.go#L14-L60
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.
Definitely agreed, but as the scope of this is limited to just the main /services page, and it only returns ServiceName and Tags, that's all I'm extending to the model here.
As we build out more of the UI that fetches and gets more properties, those'll be added to the model.
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.
For context, the total of what returns at the /services endpoint
[{
"Namespace":"default",
"Services":[{"ServiceName":"example","Tags":["foo","bar"]},{"ServiceName":"example2","Tags":["thelonious","max"]}]
}]
ui/app/models/service-fragment.js
Outdated
export default class Service extends Fragment { | ||
@attr('string') name; | ||
@attr('string') portLabel; | ||
@attr() tags; | ||
@attr('string') onUpdate; | ||
@fragment('consul-connect') connect; | ||
} |
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 consumed by TaskGroup
and in the associated Allocation
, TaskGroup
and Task
pages. I'm noticing that CI detected 19 failures and error messages failing on tests like:
allocation detail: if stopping or restarting fails, an error message is shown
.
Any idea why these tests are failing CI?
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.
Yep! Had forgotten to rename the mirage model: 3cf40b7
now settled
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.
Noticing a few places where we might be able to trim this PR. Found a duplicate template. And also noticed that the service
relationship in the job
model is unused unless I missed something.
{{page-title "Services"}} | ||
<section class="section"> | ||
<ListTable | ||
@source={{this.model.services}} | ||
as |t| | ||
> | ||
<t.head> | ||
<th> | ||
Name | ||
</th> | ||
<th> | ||
Tags | ||
</th> | ||
</t.head> | ||
<t.body as |row|> | ||
<tr data-test-service-row> | ||
<td>{{row.model.name}}</td> | ||
<td> | ||
{{#each row.model.tags as |tag|}} | ||
<span class="tag">{{tag}}</span> | ||
{{/each}} | ||
</td> | ||
</tr> | ||
</t.body> | ||
</ListTable> | ||
</section> |
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
: Is this a duplicate template? I see we have nearly the same template at templates/services/index
? Why are we adding a duplicate of this to templates/components/services.hbs
?
Closing; handled in a very different way in #14408 |
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. |
service
model toservice-fragment
model