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.

Sign up for GitHub

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: Remodel task-group-deployment-summary to properly use PlacedCanaries #4325

Merged
merged 1 commit into from
May 25, 2018
+29 −19
Merged

UI: Remodel task-group-deployment-summary to properly use PlacedCanaries #4325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 6 additions & 2 deletions ui/app/models/task-group-deployment-summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { gt } from '@ember/object/computed';
import { gt, alias } from '@ember/object/computed';
import Fragment from 'ember-data-model-fragments/fragment';
import attr from 'ember-data/attr';
import { fragmentOwner } from 'ember-data-model-fragments/attributes';
Expand All @@ -12,7 +12,11 @@ export default Fragment.extend({
promoted: attr('boolean'),
requiresPromotion: gt('desiredCanaries', 0),

placedCanaries: attr('number'),
// The list of canary allocation IDs
// hasMany is not supported in fragments
placedCanaryAllocations: attr({ defaultValue: () => [] }),

placedCanaries: alias('placedCanaryAllocations.length'),
desiredCanaries: attr('number'),
desiredTotal: attr('number'),
placedAllocs: attr('number'),
Expand Down
9 changes: 9 additions & 0 deletions ui/app/serializers/task-group-deployment-summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import ApplicationSerializer from './application';

export default ApplicationSerializer.extend({
normalize(typeHash, hash) {
hash.PlacedCanaryAllocations = hash.PlacedCanaries || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need this if you are setting the defaultValue above? (or vice versa)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaultValue isn't invoked when the value is null (as opposed to undefined), so this is needed. I suppose the defaultValue in the model isn't strictly necessary, but I like that it kinda doubles as a type annotation.

delete hash.PlacedCanaries;
return this._super(typeHash, hash);
},
});
6 changes: 5 additions & 1 deletion ui/mirage/factories/deployment-task-group-summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,12 @@ export default Factory.extend({
return faker.random.number(Math.floor(this.desiredTotal / 2));
},

// PlacedCanaries is an array of allocation IDs. Since the IDs aren't currently
// used for associating allocations, any random value will do for now.
placedCanaries() {
return faker.random.number(this.desiredCanaries);
return Array(faker.random.number(this.desiredCanaries))
.fill(null)
.map(() => faker.random.uuid());
},

placedAllocs() {
Expand Down
25 changes: 9 additions & 16 deletions ui/tests/acceptance/job-deployments-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import { test } from 'qunit';
import moment from 'moment';
import moduleForAcceptance from 'nomad-ui/tests/helpers/module-for-acceptance';

const sum = (list, key) => list.reduce((sum, item) => sum + get(item, key), 0);
const sum = (list, key, getter = a => a) =>
list.reduce((sum, item) => sum + getter(get(item, key)), 0);
Copy link

Choose a reason for hiding this comment

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

Nice helper!


let job;
let deployments;
Expand Down Expand Up @@ -40,9 +41,7 @@ test('/jobs/:id/deployments should list all job deployments', function(assert) {
});
});

test('each deployment mentions the deployment shortId, status, version, and time since it was submitted', function(
assert
) {
test('each deployment mentions the deployment shortId, status, version, and time since it was submitted', function(assert) {
visit(`/jobs/${job.id}/deployments`);

andThen(() => {
Expand Down Expand Up @@ -82,9 +81,7 @@ test('each deployment mentions the deployment shortId, status, version, and time
});
});

test('when the deployment is running and needs promotion, the deployment item says so', function(
assert
) {
test('when the deployment is running and needs promotion, the deployment item says so', function(assert) {
// Ensure the deployment needs deployment
const deployment = sortedDeployments.models[0];
const taskGroupSummary = deployment.deploymentTaskGroupSummaryIds.map(id =>
Expand All @@ -96,7 +93,7 @@ test('when the deployment is running and needs promotion, the deployment item sa

taskGroupSummary.update({
desiredCanaries: 1,
placedCanaries: 0,
placedCanaries: [],
promoted: false,
});

Expand Down Expand Up @@ -152,7 +149,7 @@ test('when open, a deployment shows the deployment metrics', function(assert) {
andThen(() => {
assert.equal(
find('[data-test-deployment-metric="canaries"]').textContent.trim(),
`${sum(taskGroupSummaries, 'placedCanaries')} / ${sum(
`${sum(taskGroupSummaries, 'placedCanaries', a => a.length)} / ${sum(
taskGroupSummaries,
'desiredCanaries'
)}`,
Expand Down Expand Up @@ -192,9 +189,7 @@ test('when open, a deployment shows the deployment metrics', function(assert) {
});
});

test('when open, a deployment shows a list of all task groups and their respective stats', function(
assert
) {
test('when open, a deployment shows a list of all task groups and their respective stats', function(assert) {
visit(`/jobs/${job.id}/deployments`);

andThen(() => {
Expand Down Expand Up @@ -241,7 +236,7 @@ test('when open, a deployment shows a list of all task groups and their respecti
);
assert.equal(
taskGroupRow.querySelector('[data-test-deployment-task-group-canaries]').textContent.trim(),
`${taskGroup.placedCanaries} / ${taskGroup.desiredCanaries}`,
`${taskGroup.placedCanaries.length} / ${taskGroup.desiredCanaries}`,
'Canaries'
);
assert.equal(
Expand All @@ -265,9 +260,7 @@ test('when open, a deployment shows a list of all task groups and their respecti
});
});

test('when open, a deployment shows a list of all allocations for the deployment', function(
assert
) {
test('when open, a deployment shows a list of all allocations for the deployment', function(assert) {
visit(`/jobs/${job.id}/deployments`);

andThen(() => {
Expand Down