From de96c3498b3867bb5adbaaad5798261260b0005c Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 29 Nov 2024 16:12:56 -0500 Subject: [PATCH] [ui] "Clone and Edit" functionality for versions (#24168) * Edit from Version functionality * Reworked as Clone and Revert * Change name warning for cloning as new job, version 0 checking, and erroring on sourceless clone * If you try to plan a new version of a job with a different name of the one you're editing, suggest they might want to run a new one instead * A few code comments and log cleanup * Scaffolding new acceptance tests * A whack of fun new tests * Unit test for version number url passing on fetchRawDef * Bit of cleanup * fetchRawDefinition gets version support at adapter layer * Handle spec-not-available-but-definition-is for clone-as-new-job --- .changelog/24168.txt | 3 + ui/app/adapters/job.js | 42 ++++- ui/app/components/job-version.js | 59 +++++++ ui/app/controllers/jobs/run/index.js | 2 +- ui/app/models/job.js | 8 +- ui/app/routes/jobs/job/definition.js | 34 +++- ui/app/routes/jobs/run/index.js | 11 +- .../templates/components/job-editor/alert.hbs | 4 +- ui/app/templates/components/job-version.hbs | 67 ++++++-- ui/app/templates/jobs/run/index.hbs | 8 +- ui/tests/acceptance/job-versions-test.js | 160 ++++++++++++++++++ ui/tests/unit/adapters/job-test.js | 80 +++++++++ 12 files changed, 446 insertions(+), 32 deletions(-) create mode 100644 .changelog/24168.txt diff --git a/.changelog/24168.txt b/.changelog/24168.txt new file mode 100644 index 00000000000..08e5bf4008b --- /dev/null +++ b/.changelog/24168.txt @@ -0,0 +1,3 @@ +```release-note:improvement +ui: Add an Edit From Version button as an option when reverting from an older job version +``` diff --git a/ui/app/adapters/job.js b/ui/app/adapters/job.js index 731724c0862..b781bf8008c 100644 --- a/ui/app/adapters/job.js +++ b/ui/app/adapters/job.js @@ -20,16 +20,48 @@ export default class JobAdapter extends WatchableNamespaceIDs { summary: '/summary', }; - fetchRawDefinition(job) { - const url = this.urlForFindRecord(job.get('id'), 'job'); - return this.ajax(url, 'GET'); + /** + * Gets the JSON definition of a job. + * Prior to Nomad 1.6, this was the only way to get job definition data. + * Now, this is included as a stringified JSON object when fetching raw specification (under .Source). + * This method is still important for backwards compatibility with older job versions, as well as a fallback + * for when fetching raw specification fails. + * @param {import('../models/job').default} job + * @param {number} version + */ + async fetchRawDefinition(job, version) { + if (version == null) { + const url = this.urlForFindRecord(job.get('id'), 'job'); + return this.ajax(url, 'GET'); + } + + // For specific versions, we need to fetch from versions endpoint, + // and then find the specified version info from the response. + const versionsUrl = addToPath( + this.urlForFindRecord(job.get('id'), 'job', null, 'versions') + ); + + const response = await this.ajax(versionsUrl, 'GET'); + const versionInfo = response.Versions.find((v) => v.Version === version); + + if (!versionInfo) { + throw new Error(`Version ${version} not found`); + } + + return versionInfo; } - fetchRawSpecification(job) { + /** + * Gets submission info for a job, including (if available) the raw HCL or JSON spec used to run it, + * including variable flags and literals. + * @param {import('../models/job').default} job + * @param {number} version + */ + fetchRawSpecification(job, version) { const url = addToPath( this.urlForFindRecord(job.get('id'), 'job', null, 'submission'), '', - 'version=' + job.get('version') + 'version=' + (version || job.get('version')) ); return this.ajax(url, 'GET'); } diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index 31606f9844e..803491011ea 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -3,6 +3,8 @@ * SPDX-License-Identifier: BUSL-1.1 */ +// @ts-check + import Component from '@glimmer/component'; import { action, computed } from '@ember/object'; import { alias } from '@ember/object/computed'; @@ -80,6 +82,11 @@ export default class JobVersion extends Component { this.isOpen = !this.isOpen; } + /** + * @type {'idle' | 'confirming'} + */ + @tracked cloneButtonStatus = 'idle'; + @task(function* () { try { const versionBeforeReversion = this.version.get('job.version'); @@ -108,6 +115,58 @@ export default class JobVersion extends Component { }) revertTo; + @action async cloneAsNewVersion() { + try { + this.router.transitionTo( + 'jobs.job.definition', + this.version.get('job.idWithNamespace'), + { + queryParams: { + isEditing: true, + version: this.version.number, + }, + } + ); + } catch (e) { + this.args.handleError({ + level: 'danger', + title: 'Could Not Edit from Version', + }); + } + } + + @action async cloneAsNewJob() { + const job = await this.version.get('job'); + try { + const specification = await job.fetchRawSpecification( + this.version.number + ); + this.router.transitionTo('jobs.run', { + queryParams: { + sourceString: specification.Source, + }, + }); + return; + } catch (specError) { + try { + // If submission info is not available, try to fetch the raw definition + const definition = await job.fetchRawDefinition(this.version.number); + this.router.transitionTo('jobs.run', { + queryParams: { + sourceString: JSON.stringify(definition, null, 2), + }, + }); + } catch (defError) { + // Both methods failed, show error + this.args.handleError({ + level: 'danger', + title: 'Could Not Clone as New Job', + description: messageForError(defError), + }); + } + } + } + @action handleKeydown(event) { if (event.key === 'Escape') { diff --git a/ui/app/controllers/jobs/run/index.js b/ui/app/controllers/jobs/run/index.js index 4c6980c99b2..47536019691 100644 --- a/ui/app/controllers/jobs/run/index.js +++ b/ui/app/controllers/jobs/run/index.js @@ -11,7 +11,7 @@ import { inject as service } from '@ember/service'; export default class RunController extends Controller { @service router; - queryParams = ['template']; + queryParams = ['template', 'sourceString', 'disregardNameWarning']; @action handleSaveAsTemplate() { diff --git a/ui/app/models/job.js b/ui/app/models/job.js index eee53d64728..007c8a699b5 100644 --- a/ui/app/models/job.js +++ b/ui/app/models/job.js @@ -537,12 +537,12 @@ export default class Job extends Model { return undefined; } - fetchRawDefinition() { - return this.store.adapterFor('job').fetchRawDefinition(this); + fetchRawDefinition(version) { + return this.store.adapterFor('job').fetchRawDefinition(this, version); } - fetchRawSpecification() { - return this.store.adapterFor('job').fetchRawSpecification(this); + fetchRawSpecification(version) { + return this.store.adapterFor('job').fetchRawSpecification(this, version); } forcePeriodic() { diff --git a/ui/app/routes/jobs/job/definition.js b/ui/app/routes/jobs/job/definition.js index 886683d6bb4..31dabec20ae 100644 --- a/ui/app/routes/jobs/job/definition.js +++ b/ui/app/routes/jobs/job/definition.js @@ -5,29 +5,55 @@ // @ts-check import Route from '@ember/routing/route'; - +import { inject as service } from '@ember/service'; /** * Route for fetching and displaying a job's definition and specification. */ export default class DefinitionRoute extends Route { + @service notifications; + + queryParams = { + version: { + refreshModel: true, + }, + }; + /** * Fetch the job's definition, specification, and variables from the API. * * @returns {Promise} A promise that resolves to an object containing the job, definition, format, * specification, variableFlags, and variableLiteral. */ - async model() { + async model({ version }) { + version = version ? +version : undefined; // query parameter is a string; convert to number + /** @type {import('../../../models/job').default} */ const job = this.modelFor('jobs.job'); if (!job) return; - const definition = await job.fetchRawDefinition(); + let definition; + + if (version !== undefined) { + // Not (!version) because version can be 0 + try { + definition = await job.fetchRawDefinition(version); + } catch (e) { + console.error('error fetching job version definition', e); + this.notifications.add({ + title: 'Error Fetching Job Version Definition', + message: `There was an error fetching the versions for this job: ${e.message}`, + color: 'critical', + }); + } + } else { + definition = await job.fetchRawDefinition(); + } let format = 'json'; // default to json in network request errors let specification; let variableFlags; let variableLiteral; try { - const specificationResponse = await job.fetchRawSpecification(); + const specificationResponse = await job.fetchRawSpecification(version); specification = specificationResponse?.Source ?? null; variableFlags = specificationResponse?.VariableFlags ?? null; variableLiteral = specificationResponse?.Variables ?? null; diff --git a/ui/app/routes/jobs/run/index.js b/ui/app/routes/jobs/run/index.js index 95c0bbffd2d..38e11617335 100644 --- a/ui/app/routes/jobs/run/index.js +++ b/ui/app/routes/jobs/run/index.js @@ -21,6 +21,9 @@ export default class JobsRunIndexRoute extends Route { template: { refreshModel: true, }, + sourceString: { + refreshModel: true, + }, }; beforeModel(transition) { @@ -33,7 +36,7 @@ export default class JobsRunIndexRoute extends Route { } } - async model({ template }) { + async model({ template, sourceString }) { try { // When jobs are created with a namespace attribute, it is verified against // available namespaces to prevent redirecting to a non-existent namespace. @@ -45,6 +48,10 @@ export default class JobsRunIndexRoute extends Route { return this.store.createRecord('job', { _newDefinition: templateRecord.items.template, }); + } else if (sourceString) { + return this.store.createRecord('job', { + _newDefinition: sourceString, + }); } else { return this.store.createRecord('job'); } @@ -72,6 +79,8 @@ export default class JobsRunIndexRoute extends Route { if (isExiting) { controller.model?.deleteRecord(); controller.set('template', null); + controller.set('sourceString', null); + controller.set('disregardNameWarning', null); } } } diff --git a/ui/app/templates/components/job-editor/alert.hbs b/ui/app/templates/components/job-editor/alert.hbs index f3f88c9ccfb..38159998a80 100644 --- a/ui/app/templates/components/job-editor/alert.hbs +++ b/ui/app/templates/components/job-editor/alert.hbs @@ -8,6 +8,9 @@ {{conditionally-capitalize @data.error.type true}} {{@data.error.message}} + {{#if (eq @data.error.message "Job ID does not match")}} + + {{/if}} {{/if}} {{#if (and (eq @data.stage "read") @data.hasVariables (eq @data.view "job-spec"))}} @@ -31,4 +34,3 @@ {{/if}} - \ No newline at end of file diff --git a/ui/app/templates/components/job-version.hbs b/ui/app/templates/components/job-version.hbs index 587bb18c978..fbc6e46d753 100644 --- a/ui/app/templates/components/job-version.hbs +++ b/ui/app/templates/components/job-version.hbs @@ -4,7 +4,7 @@ ~}} {{did-update this.versionsDidUpdate this.diff}} -
+
Version #{{this.version.number}} @@ -81,20 +81,57 @@
{{#unless this.isCurrent}} {{#if (can "run job" namespace=this.version.job.namespace)}} - + {{#if (eq this.cloneButtonStatus 'idle')}} + + + + {{else if (eq this.cloneButtonStatus 'confirming')}} + + + + {{/if}} {{else}} {{page-title "Run a job"}}
+ {{#if (and this.sourceString (not this.disregardNameWarning))}} + + Don't forget to change the job name! + Since you're cloning a job version's source as a new job, you'll want to change the job name. Otherwise, this will appear as a new version of the original job, rather than a new job. + + {{/if}} -
\ No newline at end of file +
diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index b16ac6eda88..56b4ad623cd 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -322,6 +322,166 @@ module('Acceptance | job versions', function (hooks) { }); }); +// Module for Clone and Edit +module('Acceptance | job versions (clone and edit)', function (hooks) { + setupApplicationTest(hooks); + setupMirage(hooks); + + hooks.beforeEach(async function () { + server.create('node-pool'); + namespace = server.create('namespace'); + + const managementToken = server.create('token'); + window.localStorage.nomadTokenSecret = managementToken.secretId; + + job = server.create('job', { + createAllocations: false, + version: 99, + namespaceId: namespace.id, + }); + // remove auto-created versions and create 3 of them, one with a tag + server.db.jobVersions.remove(); + server.create('job-version', { + job, + version: 99, + submitTime: 1731101785761339000, + }); + server.create('job-version', { + job, + version: 98, + submitTime: 1731101685761339000, + versionTag: { + Name: 'test-tag', + Description: 'A tag with a brief description', + }, + }); + server.create('job-version', { + job, + version: 0, + submitTime: 1731101585761339000, + }); + await Versions.visit({ id: job.id }); + }); + + test('Clone and edit buttons are shown', async function (assert) { + assert.dom('.job-version').exists({ count: 3 }); + assert + .dom('[data-test-clone-and-edit]') + .exists( + { count: 2 }, + 'Current job version doesnt have clone or revert buttons' + ); + + const versionBlock = '[data-test-job-version="98"]'; + + assert + .dom(`${versionBlock} [data-test-clone-as-new-version]`) + .doesNotExist( + 'Confirmation-stage clone-as-new-version button doesnt exist on initial load' + ); + assert + .dom(`${versionBlock} [data-test-clone-as-new-job]`) + .doesNotExist( + 'Confirmation-stage clone-as-new-job button doesnt exist on initial load' + ); + + await click(`${versionBlock} [data-test-clone-and-edit]`); + + assert + .dom(`${versionBlock} [data-test-clone-as-new-version]`) + .exists( + 'Confirmation-stage clone-as-new-version button exists after clicking clone and edit' + ); + + assert + .dom(`${versionBlock} [data-test-clone-as-new-job]`) + .exists( + 'Confirmation-stage clone-as-new-job button exists after clicking clone and edit' + ); + + assert + .dom(`${versionBlock} [data-test-revert-to]`) + .doesNotExist('Revert button is hidden when Clone buttons are expanded'); + + assert + .dom('[data-test-job-version="0"] [data-test-revert-to]') + .exists('Revert button is visible for other versions'); + + await click(`${versionBlock} [data-test-cancel-clone]`); + + assert + .dom(`${versionBlock} [data-test-clone-as-new-version]`) + .doesNotExist( + 'Confirmation-stage clone-as-new-version button doesnt exist after clicking cancel' + ); + }); + + test('Clone as new version', async function (assert) { + const versionBlock = '[data-test-job-version="98"]'; + await click(`${versionBlock} [data-test-clone-and-edit]`); + await click(`${versionBlock} [data-test-clone-as-new-version]`); + + assert.equal( + currentURL(), + `/jobs/${job.id}@${namespace.id}/definition?isEditing=true&version=98&view=job-spec`, + 'Taken to the definition page in edit mode' + ); + + await percySnapshot(assert); + }); + + test('Clone as new version when version is 0', async function (assert) { + const versionBlock = '[data-test-job-version="0"]'; + await click(`${versionBlock} [data-test-clone-and-edit]`); + await click(`${versionBlock} [data-test-clone-as-new-version]`); + + assert.equal( + currentURL(), + `/jobs/${job.id}@${namespace.id}/definition?isEditing=true&version=0&view=job-spec`, + 'Taken to the definition page in edit mode' + ); + }); + + test('Clone as a new version when no submission info is available', async function (assert) { + server.pretender.get('/v1/job/:id/submission', () => [500, {}, '']); + const versionBlock = '[data-test-job-version="98"]'; + await click(`${versionBlock} [data-test-clone-and-edit]`); + await click(`${versionBlock} [data-test-clone-as-new-version]`); + + assert.equal( + currentURL(), + `/jobs/${job.id}@${namespace.id}/definition?isEditing=true&version=98&view=full-definition`, + 'Taken to the definition page in edit mode' + ); + + assert.dom('[data-test-json-warning]').exists(); + + await percySnapshot(assert); + }); + + test('Clone as a new job', async function (assert) { + const testString = + 'Test string that should appear in my sourceString url param'; + server.pretender.get('/v1/job/:id/submission', () => [ + 200, + {}, + JSON.stringify({ + Source: testString, + }), + ]); + const versionBlock = '[data-test-job-version="98"]'; + await click(`${versionBlock} [data-test-clone-and-edit]`); + await click(`${versionBlock} [data-test-clone-as-new-job]`); + + assert.equal( + currentURL(), + `/jobs/run?sourceString=${encodeURIComponent(testString)}`, + 'Taken to the new job page' + ); + assert.dom('[data-test-job-name-warning]').exists(); + }); +}); + module('Acceptance | job versions (with client token)', function (hooks) { setupApplicationTest(hooks); setupMirage(hooks); diff --git a/ui/tests/unit/adapters/job-test.js b/ui/tests/unit/adapters/job-test.js index d6437ac0e21..370573113c7 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -501,6 +501,65 @@ module('Unit | Adapter | Job', function (hooks) { assert.equal(request.method, 'GET'); }); + test('fetchRawDefinition handles version requests', async function (assert) { + assert.expect(5); + + const adapter = this.owner.lookup('adapter:job'); + const job = { + get: sinon.stub(), + }; + + job.get.withArgs('id').returns('["job-id"]'); + + const mockVersionResponse = { + Versions: [ + { Version: 1, JobID: 'job-id', JobModifyIndex: 100 }, + { Version: 2, JobID: 'job-id', JobModifyIndex: 200 }, + ], + }; + + // Stub ajax to return mock version data + const ajaxStub = sinon.stub(adapter, 'ajax'); + ajaxStub + .withArgs('/v1/job/job-id/versions', 'GET') + .resolves(mockVersionResponse); + + // Test fetching specific version + const result = await adapter.fetchRawDefinition(job, 2); + assert.equal(result.Version, 2, 'Returns correct version'); + assert.equal(result.JobModifyIndex, 200, 'Returns full version info'); + + // Test version not found + try { + await adapter.fetchRawDefinition(job, 999); + assert.ok(false, 'Should have thrown error'); + } catch (e) { + assert.equal( + e.message, + 'Version 999 not found', + 'Throws appropriate error' + ); + } + + // Test no version specified (current version) + ajaxStub + .withArgs('/v1/job/job-id', 'GET') + .resolves({ ID: 'job-id', Version: 2 }); + + const currentResult = await adapter.fetchRawDefinition(job); + + assert.equal( + ajaxStub.lastCall.args[0], + '/v1/job/job-id', + 'URL has no version query param' + ); + assert.equal( + currentResult.Version, + 2, + 'Returns current version when no version specified' + ); + }); + test('forcePeriodic requests include the activeRegion', async function (assert) { const region = 'region-2'; const job = await this.initializeWithJob({ region }); @@ -681,6 +740,27 @@ module('Unit | Adapter | Job', function (hooks) { '/v1/job/job-id/submission?namespace=zoey&version=job-version' ); }); + test('Requests for specific versions include the queryParam', async function (assert) { + const adapter = this.owner.lookup('adapter:job'); + const job = { + get: sinon.stub(), + }; + job.get.withArgs('id').returns('["job-id"]'); + job.get.withArgs('version').returns(100); + + // Stub the ajax method to avoid making real API calls + sinon.stub(adapter, 'ajax').callsFake(() => resolve({})); + + await adapter.fetchRawSpecification(job, 99); + + assert.ok(adapter.ajax.calledOnce, 'The ajax method is called once'); + assert.equal( + adapter.ajax.args[0][0], + '/v1/job/job-id/submission?version=99', + 'it includes the version query param' + ); + assert.equal(adapter.ajax.args[0][1], 'GET'); + }); }); });