Skip to content
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

Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ui/app/models/job.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ export default class Job extends Model {

@hasMany('recommendation-summary') recommendationSummaries;

@hasMany('services') services;

@computed('taskGroups.@each.drivers')
get drivers() {
return this.taskGroups
Expand Down
11 changes: 11 additions & 0 deletions ui/app/models/service-fragment.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { attr } from '@ember-data/model';
import Fragment from 'ember-data-model-fragments/fragment';
import { fragment } from 'ember-data-model-fragments/attributes';

export default class Service extends Fragment {
@attr('string') name;
@attr('string') portLabel;
@attr() tags;
@attr('string') onUpdate;
@fragment('consul-connect') connect;
}
Copy link
Contributor

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?

Copy link
Contributor Author

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

13 changes: 6 additions & 7 deletions ui/app/models/service.js
Original file line number Diff line number Diff line change
@@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Unused import.

import Model from '@ember-data/model';
import { alias } from '@ember/object/computed';

export default class Service extends Fragment {
@attr('string') name;
@attr('string') portLabel;
export default class Service extends Model {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@philrenaud philrenaud Jul 21, 2022

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"]}]
}]

@attr('string') ServiceName;
Copy link
Contributor

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?

@alias('ServiceName') name;
@attr() tags;
@attr('string') onUpdate;
@fragment('consul-connect') connect;
}
2 changes: 1 addition & 1 deletion ui/app/models/task-group.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export default class TaskGroup extends Fragment {

@fragmentArray('task') tasks;

@fragmentArray('service') services;
@fragmentArray('service-fragment') services;
Copy link
Contributor

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?


@fragmentArray('volume-definition') volumes;

Expand Down
1 change: 1 addition & 0 deletions ui/app/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,5 @@ Router.map(function () {
path: '/path/*absolutePath',
});
});
this.route('services', function () {});
});
18 changes: 18 additions & 0 deletions ui/app/routes/services.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { inject as service } from '@ember/service';
import Route from '@ember/routing/route';
import RSVP from 'rsvp';
import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state';
import notifyForbidden from 'nomad-ui/utils/notify-forbidden';
import classic from 'ember-classic-decorator';

@classic
export default class ServicesRoute extends Route.extend(WithForbiddenState) {
@service store;
@service system;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Where are we using the system service here?


async model() {
return RSVP.hash({
services: this.store.findAll('service'),
}).catch(notifyForbidden(this));
Comment on lines +14 to +16
Copy link
Contributor

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.

}
}
3 changes: 3 additions & 0 deletions ui/app/routes/services/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import Route from '@ember/routing/route';

export default class ServicesIndexRoute extends Route {}
11 changes: 11 additions & 0 deletions ui/app/serializers/service-fragment.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import ApplicationSerializer from './application';
import classic from 'ember-classic-decorator';

@classic
export default class ServiceFragmentSerializer extends ApplicationSerializer {
attrs = {
connect: 'Connect',
};

arrayNullOverrides = ['Tags'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Be careful here. Some times the API might throw you an object back like was the case with Variables and Evaluations.

}
17 changes: 12 additions & 5 deletions ui/app/serializers/service.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,16 @@ import classic from 'ember-classic-decorator';

@classic
export default class ServiceSerializer extends ApplicationSerializer {
attrs = {
connect: 'Connect',
};

arrayNullOverrides = ['Tags'];
normalize(typeHash, hash) {
hash.ID = hash.ServiceName;
return super.normalize(typeHash, hash);
}
normalizeResponse(store, typeClass, hash, ...args) {
return super.normalizeResponse(
store,
typeClass,
hash.mapBy('Services').flat() || [],
...args
);
}
}
9 changes: 9 additions & 0 deletions ui/app/templates/components/gutter-menu.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,15 @@
</LinkTo>
</li>
{{/if}}
<li>
<LinkTo
@route="services"
@activeClass="is-active"
data-test-gutter-link="services"
>
Services
</LinkTo>
</li>
</ul>
<p class="menu-label">
Cluster
Expand Down
4 changes: 4 additions & 0 deletions ui/app/templates/services.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<Breadcrumb @crumb={{hash label="Services" args=(array "services.index")}} />
<PageLayout>
{{outlet}}
</PageLayout>
26 changes: 26 additions & 0 deletions ui/app/templates/services/index.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{{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>
Comment on lines +1 to +26
Copy link
Contributor

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?

11 changes: 11 additions & 0 deletions ui/mirage/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,17 @@ export default function () {
});

//#endregion Secure Variables

//#region Services
this.get('/services', function (schema) {
return [
{
Namespace: 'default',
Copy link
Contributor

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.

Services: schema.services.all().models,
},
];
});
//#endregion Services
}

function filterKeys(object, ...keys) {
Expand Down
34 changes: 34 additions & 0 deletions ui/mirage/factories/service-fragment.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { Factory } from 'ember-cli-mirage';
import faker from 'nomad-ui/mirage/faker';
import { provide } from '../utils';
import { dasherize } from '@ember/string';

const ON_UPDATE = ['default', 'ignore', 'ignore_warnings'];

export default Factory.extend({
name: (id) => `${dasherize(faker.hacker.noun())}-${id}-service`,
portLabel: () => dasherize(faker.hacker.noun()),
onUpdate: faker.helpers.randomize(ON_UPDATE),
tags: () => {
if (!faker.random.boolean()) {
return provide(
faker.random.number({ min: 0, max: 2 }),
faker.hacker.noun.bind(faker.hacker.noun)
);
} else {
return null;
}
},
Connect: {
SidecarService: {
Proxy: {
Upstreams: [
{
DestinationName: dasherize(faker.hacker.noun()),
LocalBindPort: faker.random.number({ min: 5000, max: 60000 }),
},
],
},
},
},
});
22 changes: 3 additions & 19 deletions ui/mirage/factories/service.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
import { Factory } from 'ember-cli-mirage';
import faker from 'nomad-ui/mirage/faker';
import { pickOne } from '../utils';
import { provide } from '../utils';
Comment on lines +3 to 4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Duplicate import path.

import { dasherize } from '@ember/string';

const ON_UPDATE = ['default', 'ignore', 'ignore_warnings'];

export default Factory.extend({
name: (id) => `${dasherize(faker.hacker.noun())}-${id}-service`,
portLabel: () => dasherize(faker.hacker.noun()),
onUpdate: faker.helpers.randomize(ON_UPDATE),
tags: () => {
ServiceName: () => `${faker.hacker.adjective()}-${faker.hacker.noun()}`,
Tags: () => {
Comment on lines +7 to +8
Copy link
Contributor

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?

if (!faker.random.boolean()) {
return provide(
faker.random.number({ min: 0, max: 2 }),
Expand All @@ -19,16 +15,4 @@ export default Factory.extend({
return null;
}
},
Connect: {
SidecarService: {
Proxy: {
Upstreams: [
{
DestinationName: dasherize(faker.hacker.noun()),
LocalBindPort: faker.random.number({ min: 5000, max: 60000 }),
},
],
},
},
},
});
2 changes: 1 addition & 1 deletion ui/mirage/factories/task-group.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ export default Factory.extend({
Array(faker.random.number({ min: 1, max: 3 }))
.fill(null)
.forEach(() => {
server.create('service', {
server.create('service-fragment', {
taskGroup: group,
});
});
Expand Down
2 changes: 1 addition & 1 deletion ui/mirage/models/task-group.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ import { Model, belongsTo, hasMany } from 'ember-cli-mirage';

export default Model.extend({
job: belongsTo(),
services: hasMany(),
services: hasMany('service-fragment'),
tasks: hasMany(),
});
1 change: 1 addition & 0 deletions ui/mirage/scenarios/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ function smallCluster(server) {
server.create('allocFile', 'dir', { depth: 2 });
server.createList('csi-plugin', 2);
server.createList('variable', 3);
server.createList('service', 10);

const variableLinkedJob = server.db.jobs[0];
const variableLinkedGroup = server.db.taskGroups.findBy({
Expand Down
50 changes: 50 additions & 0 deletions ui/tests/acceptance/services-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { module, test } from 'qunit';
import { visit, currentURL, findAll } from '@ember/test-helpers';
import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
import percySnapshot from '@percy/ember';
import Services from 'nomad-ui/tests/pages/services';
import Layout from 'nomad-ui/tests/pages/layout';
import defaultScenario from '../../mirage/scenarios/default';
import a11yAudit from 'nomad-ui/tests/helpers/a11y-audit';

module('Acceptance | services', function (hooks) {
setupApplicationTest(hooks);
setupMirage(hooks);

test('it passes an accessibility audit', async function (assert) {
assert.expect(1);
defaultScenario(server);
await Services.visit();
await a11yAudit(assert);
});

module('traversal', function () {
test('visiting /services by url', async function (assert) {
defaultScenario(server);
await Services.visit();
assert.equal(currentURL(), '/services');
});

test('main menu correctly takes you to services', async function (assert) {
assert.expect(1);
defaultScenario(server);
await visit('/');
await Layout.gutter.visitServices();
assert.equal(currentURL(), '/services');
await percySnapshot(assert);
});
});

module('services index table', function () {
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'
);
Comment on lines +40 to +47
Copy link
Contributor

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.

});
});
});
1 change: 1 addition & 0 deletions ui/tests/pages/layout.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ export default create({
visitClients: clickable('[data-test-gutter-link="clients"]'),
visitServers: clickable('[data-test-gutter-link="servers"]'),
visitStorage: clickable('[data-test-gutter-link="storage"]'),
visitServices: clickable('[data-test-gutter-link="services"]'),
},

breadcrumbs: collection('[data-test-breadcrumb]', {
Expand Down
5 changes: 5 additions & 0 deletions ui/tests/pages/services.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { create, visitable } from 'ember-cli-page-object';

export default create({
visit: visitable('/services'),
});