Add job version revert buttons (#10336)

This adds a Revert two-step button to the JobVersions component for
not-current versions, which redirects to the overview on success. It
checks the job version before and after reversion to mitigate the edge
case where reverting to an otherwise-identical version has no effect, as
discussed in #10337.

It uses existing facilities for handling other errors and disabling the
button when permissions are lacking.
This commit is contained in:
Buck Doyle
2021-04-20 08:33:16 -05:00
committed by GitHub
parent 0f4027df45
commit d0f4750f51
16 changed files with 303 additions and 16 deletions

View File

@@ -23,6 +23,7 @@ IMPROVEMENTS:
* networking: Added support for user-defined iptables rules on the NOMAD-ADMIN chain. [[GH-10181](https://github.com/hashicorp/nomad/issues/10181)]
* networking: Added support for interpolating host network names with node attributes. [[GH-10196](https://github.com/hashicorp/nomad/issues/10196)]
* nomad/structs: Removed deprecated Node.Drain field, added API extensions to restore it [[GH-10202](https://github.com/hashicorp/nomad/issues/10202)]
* ui: Added a job reversion button [[GH-10336](https://github.com/hashicorp/nomad/pull/10336)]
BUG FIXES:
* core (Enterprise): Update licensing library to v0.0.11 to include race condition fix. [[GH-10253](https://github.com/hashicorp/nomad/issues/10253)]

View File

@@ -0,0 +1,18 @@
import ApplicationAdapter from './application';
import addToPath from 'nomad-ui/utils/add-to-path';
export default class JobVersionAdapter extends ApplicationAdapter {
revertTo(jobVersion) {
const jobAdapter = this.store.adapterFor('job');
const url = addToPath(jobAdapter.urlForFindRecord(jobVersion.get('job.id'), 'job'), '/revert');
const [jobName] = JSON.parse(jobVersion.get('job.id'));
return this.ajax(url, 'POST', {
data: {
JobID: jobName,
JobVersion: jobVersion.number,
},
});
}
}

View File

@@ -1,6 +1,9 @@
import Component from '@ember/component';
import { action, computed } from '@ember/object';
import { classNames } from '@ember-decorators/component';
import { inject as service } from '@ember/service';
import { task } from 'ember-concurrency';
import messageForError from 'nomad-ui/utils/message-from-adapter-error';
import classic from 'ember-classic-decorator';
const changeTypes = ['Added', 'Deleted', 'Edited'];
@@ -14,6 +17,8 @@ export default class JobVersion extends Component {
// Passes through to the job-diff component
verbose = true;
@service router;
@computed('version.diff')
get changeCount() {
const diff = this.get('version.diff');
@@ -30,10 +35,47 @@ export default class JobVersion extends Component {
);
}
@computed('version.{number,job.version}')
get isCurrent() {
return this.get('version.number') === this.get('version.job.version');
}
@action
toggleDiff() {
this.toggleProperty('isOpen');
}
@task(function*() {
try {
const versionBeforeReversion = this.get('version.job.version');
yield this.version.revertTo();
yield this.version.job.reload();
const versionAfterReversion = this.get('version.job.version');
if (versionBeforeReversion === versionAfterReversion) {
this.handleError({
level: 'warn',
title: 'Reversion Had No Effect',
description: 'Reverting to an identical older version doesnt produce a new version',
});
} else {
const job = this.get('version.job');
this.router.transitionTo('jobs.job', job.get('plainId'), {
queryParams: { namespace: job.get('namespace.name') },
});
}
} catch (e) {
this.handleError({
level: 'danger',
title: 'Could Not Revert',
description: messageForError(e, 'revert'),
});
}
})
revertTo;
}
const flatten = (accumulator, array) => accumulator.concat(array);

View File

@@ -9,7 +9,7 @@ import classic from 'ember-classic-decorator';
@classic
@classNames('two-step-button')
@classNameBindings('inlineText:has-inline-text')
@classNameBindings('inlineText:has-inline-text', 'fadingBackground:has-fading-background')
export default class TwoStepButton extends Component {
idleText = '';
cancelText = '';

View File

@@ -1,9 +1,33 @@
import Controller from '@ember/controller';
import WithNamespaceResetting from 'nomad-ui/mixins/with-namespace-resetting';
import { alias } from '@ember/object/computed';
import { action, computed } from '@ember/object';
import classic from 'ember-classic-decorator';
const alertClassFallback = 'is-info';
const errorLevelToAlertClass = {
danger: 'is-danger',
warn: 'is-warning',
};
@classic
export default class VersionsController extends Controller.extend(WithNamespaceResetting) {
error = null;
@alias('model') job;
@computed('error.level')
get errorLevelClass() {
return errorLevelToAlertClass[this.get('error.level')] || alertClassFallback;
}
onDismiss() {
this.set('error', null);
}
@action
handleError(errorObject) {
this.set('error', errorObject);
}
}

View File

@@ -7,4 +7,8 @@ export default class JobVersion extends Model {
@attr('date') submitTime;
@attr('number') number;
@attr() diff;
revertTo() {
return this.store.adapterFor('job-version').revertTo(this);
}
}

View File

@@ -1,6 +1,6 @@
import Route from '@ember/routing/route';
import { collect } from '@ember/object/computed';
import { watchRelationship } from 'nomad-ui/utils/properties/watch';
import { watchRecord, watchRelationship } from 'nomad-ui/utils/properties/watch';
import WithWatchers from 'nomad-ui/mixins/with-watchers';
export default class VersionsRoute extends Route.extend(WithWatchers) {
@@ -11,10 +11,13 @@ export default class VersionsRoute extends Route.extend(WithWatchers) {
startWatchers(controller, model) {
if (model) {
controller.set('watcher', this.watchVersions.perform(model));
controller.set('watcher', this.watch.perform(model));
controller.set('watchVersions', this.watchVersions.perform(model));
}
}
@watchRecord('job') watch;
@watchRelationship('versions') watchVersions;
@collect('watchVersions') watchers;
@collect('watch', 'watchVersions') watchers;
}

View File

@@ -19,6 +19,11 @@
margin-left: auto;
}
.is-fixed-width {
display: inline-block;
width: 8em;
}
&.is-compact {
padding: 0.75em;
}

View File

@@ -14,11 +14,19 @@
}
.confirmation-text {
position: static;
margin-right: 0.5ch;
position: absolute;
left: auto;
right: 0;
top: auto;
margin-right: 100%;
}
}
&.has-fading-background .confirmation-text {
padding: 0.5rem 0 0.5rem 4rem;
background: linear-gradient(to left, white, white 90%, transparent 100%);
}
.confirmation-text {
position: absolute;
left: 0;

View File

@@ -8,11 +8,42 @@
<span class="term">Submitted</span>
<span data-test-version-submit-time class="submit-date">{{format-ts this.version.submitTime}}</span>
</span>
{{#if this.version.diff}}
<button class="button is-light is-compact pull-right" {{action "toggleDiff"}} type="button">{{this.changeCount}} {{pluralize "Change" this.changeCount}}</button>
{{else}}
<span class="pull-right">No Changes</span>
{{/if}}
<div class="pull-right">
{{#unless this.isCurrent}}
{{#if (can "run job")}}
<TwoStepButton
data-test-revert-to
@classes={{hash
idleButton="is-warning is-outlined"
confirmButton="is-warning"}}
@alignRight={{true}}
@idleText="Revert"
@cancelText="Cancel"
@confirmText="Yes, Revert"
@confirmationMessage="Are you sure you want to revert to this version?"
@inlineText={{true}}
@fadingBackground={{true}}
@awaitingConfirmation={{this.revertTo.isRunning}}
@onConfirm={{perform this.revertTo}} />
{{else}}
<button
data-test-revert-to
type="button"
class="button is-warning is-outlined is-small tooltip"
disabled
aria-label="You dont have permission to revert"
>
Revert
</button>
{{/if}}
{{/unless}}
{{#if this.version.diff}}
<button class="button is-light is-small is-fixed-width" {{action "toggleDiff"}} type="button">{{this.changeCount}} {{pluralize "Change" this.changeCount}}</button>
{{else}}
<div class="is-fixed-width is-size-7 has-text-centered">No Changes</div>
{{/if}}
</div>
</div>
{{#if this.isOpen}}
<div class="boxed-section-body is-dark">

View File

@@ -5,6 +5,6 @@
</li>
{{/if}}
<li data-test-version class="timeline-object">
<JobVersion @version={{record.version}} @verbose={{this.verbose}} />
<JobVersion @version={{record.version}} @verbose={{this.verbose}} @handleError={{@handleError}} />
</li>
{{/each}}

View File

@@ -1,5 +1,19 @@
{{page-title "Job " this.job.name " versions"}}
<JobSubnav @job={{this.job}} />
<section class="section">
<JobVersionsStream @versions={{this.model.versions}} @verbose={{true}} />
{{#if this.error}}
<div data-test-inline-error class="notification {{this.errorLevelClass}}">
<div class="columns">
<div class="column">
<h3 data-test-inline-error-title class="title is-4">{{this.error.title}}</h3>
<p data-test-inline-error-body>{{this.error.description}}</p>
</div>
<div class="column is-centered is-minimum">
<button data-test-inline-error-close class="button {{this.errorLevelClass}}" onclick={{action this.onDismiss}} type="button">Okay</button>
</div>
</div>
</div>
{{/if}}
<JobVersionsStream @versions={{this.model.versions}} @verbose={{true}} @handleError={{action this.handleError}} />
</section>

View File

@@ -180,6 +180,15 @@ export default function() {
return okEmpty();
});
this.post('/job/:id/revert', function({ jobs }, { requestBody }) {
const { JobID, JobVersion } = JSON.parse(requestBody);
const job = jobs.find(JobID);
job.version = JobVersion;
job.save();
return okEmpty();
});
this.post('/job/:id/scale', function({ jobs }, { params }) {
return this.serialize(jobs.find(params.id));
});

View File

@@ -4,9 +4,11 @@ import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
import a11yAudit from 'nomad-ui/tests/helpers/a11y-audit';
import Versions from 'nomad-ui/tests/pages/jobs/job/versions';
import Layout from 'nomad-ui/tests/pages/layout';
import moment from 'moment';
let job;
let namespace;
let versions;
module('Acceptance | job versions', function(hooks) {
@@ -14,10 +16,16 @@ module('Acceptance | job versions', function(hooks) {
setupMirage(hooks);
hooks.beforeEach(async function() {
job = server.create('job', { createAllocations: false });
server.create('namespace');
namespace = server.create('namespace');
job = server.create('job', { namespaceId: namespace.id, createAllocations: false });
versions = server.db.jobVersions.where({ jobId: job.id });
await Versions.visit({ id: job.id });
const managementToken = server.create('token');
window.localStorage.nomadTokenSecret = managementToken.secretId;
await Versions.visit({ id: job.id, namespace: namespace.id });
});
test('it passes an accessibility audit', async function(assert) {
@@ -41,6 +49,79 @@ module('Acceptance | job versions', function(hooks) {
assert.equal(versionRow.submitTime, formattedSubmitTime, 'Submit time');
});
test('all versions but the current one have a button to revert to that version', async function(assert) {
let versionRowToRevertTo;
Versions.versions.forEach((versionRow) => {
if (versionRow.number === job.version) {
assert.ok(versionRow.revertToButton.isHidden);
} else {
assert.ok(versionRow.revertToButton.isPresent);
versionRowToRevertTo = versionRow;
}
});
if (versionRowToRevertTo) {
const versionNumberRevertingTo = versionRowToRevertTo.number;
await versionRowToRevertTo.revertToButton.idle();
await versionRowToRevertTo.revertToButton.confirm();
const revertRequest = this.server.pretender.handledRequests.find(request => request.url.includes('revert'));
assert.equal(revertRequest.url, `/v1/job/${job.id}/revert?namespace=${namespace.id}`);
assert.deepEqual(JSON.parse(revertRequest.requestBody), {
JobID: job.id,
JobVersion: versionNumberRevertingTo,
});
assert.equal(currentURL(), `/jobs/${job.id}?namespace=${namespace.id}`);
}
});
test('when reversion fails, the error message from the API is piped through to the alert', async function(assert) {
const versionRowToRevertTo = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0];
if (versionRowToRevertTo) {
const message = 'A plaintext error message';
server.pretender.post('/v1/job/:id/revert', () => [500, {}, message]);
await versionRowToRevertTo.revertToButton.idle();
await versionRowToRevertTo.revertToButton.confirm();
assert.ok(Layout.inlineError.isShown);
assert.ok(Layout.inlineError.isDanger);
assert.ok(Layout.inlineError.title.includes('Could Not Revert'));
assert.equal(Layout.inlineError.message, message);
await Layout.inlineError.dismiss();
assert.notOk(Layout.inlineError.isShown);
} else {
assert.expect(0);
}
});
test('when reversion has no effect, the error message explains', async function(assert) {
const versionRowToRevertTo = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0];
if (versionRowToRevertTo) {
// The default Mirage implementation updates the job version as passed in, this does nothing
server.pretender.post('/v1/job/:id/revert', () => [200, {}, '']);
await versionRowToRevertTo.revertToButton.idle();
await versionRowToRevertTo.revertToButton.confirm();
assert.ok(Layout.inlineError.isShown);
assert.ok(Layout.inlineError.isWarning);
assert.ok(Layout.inlineError.title.includes('Reversion Had No Effect'));
assert.equal(Layout.inlineError.message, 'Reverting to an identical older version doesnt produce a new version');
} else {
assert.expect(0);
}
});
test('when the job for the versions is not found, an error message is shown, but the URL persists', async function(assert) {
await Versions.visit({ id: 'not-a-real-job' });
@@ -56,3 +137,31 @@ module('Acceptance | job versions', function(hooks) {
assert.equal(Versions.error.title, 'Not Found', 'Error message is for 404');
});
});
module('Acceptance | job versions (with client token)', function(hooks) {
setupApplicationTest(hooks);
setupMirage(hooks);
hooks.beforeEach(async function() {
job = server.create('job', { createAllocations: false });
versions = server.db.jobVersions.where({ jobId: job.id });
server.create('token');
const clientToken = server.create('token');
window.localStorage.nomadTokenSecret = clientToken.secretId;
await Versions.visit({ id: job.id });
});
test('reversion buttons are disabled when the token lacks permissions', async function(assert) {
const versionRowWithReversion = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0];
if (versionRowWithReversion) {
assert.ok(versionRowWithReversion.revertToButtonIsDisabled);
} else {
assert.expect(0);
}
window.localStorage.clear();
});
});

View File

@@ -1,5 +1,7 @@
import { create, collection, text, visitable } from 'ember-cli-page-object';
import { attribute, create, collection, text, visitable } from 'ember-cli-page-object';
import { getter } from 'ember-cli-page-object/macros';
import twoStepButton from 'nomad-ui/tests/pages/components/two-step-button';
import error from 'nomad-ui/tests/pages/components/error';
export default create({
@@ -9,6 +11,13 @@ export default create({
text: text(),
stability: text('[data-test-version-stability]'),
submitTime: text('[data-test-version-submit-time]'),
revertToButton: twoStepButton('[data-test-revert-to]'),
revertToButtonIsDisabled: attribute('disabled', '[data-test-revert-to]'),
number: getter(function() {
return parseInt(this.text.match(/#(\d+)/)[1]);
}),
}),
error: error(),

View File

@@ -99,4 +99,14 @@ export default create({
title: text('[data-test-error-title]'),
message: text('[data-test-error-message]'),
},
inlineError: {
isShown: isPresent('[data-test-inline-error]'),
title: text('[data-test-inline-error-title]'),
message: text('[data-test-inline-error-body]'),
dismiss: clickable('[data-test-inline-error-close]'),
isDanger: hasClass('is-danger', '[data-test-inline-error]'),
isWarning: hasClass('is-warning', '[data-test-inline-error]'),
},
});