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 1d195876cdd..c4e6e2a0096 100644 --- a/ui/app/adapters/job.js +++ b/ui/app/adapters/job.js @@ -20,16 +20,30 @@ export default class JobAdapter extends WatchableNamespaceIDs { summary: '/summary', }; + /** + * 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 + */ fetchRawDefinition(job) { const url = this.urlForFindRecord(job.get('id'), 'job'); return this.ajax(url, 'GET'); } - 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..b32b7927c49 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,46 @@ 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() { + console.log('cloneAsNewJob'); + try { + let job = await this.version.get('job'); + let specification = await job.fetchRawSpecification(this.version.number); + let specificationSourceString = specification.Source; + this.router.transitionTo('jobs.run', { + queryParams: { + sourceString: specificationSourceString, + }, + }); + } catch (e) { + this.args.handleError({ + level: 'danger', + title: 'Could Not Clone as New Job', + description: messageForError(e), + }); + } + } + @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..cca22f8fcaa 100644 --- a/ui/app/models/job.js +++ b/ui/app/models/job.js @@ -541,8 +541,8 @@ export default class Job extends Model { return this.store.adapterFor('job').fetchRawDefinition(this); } - 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..84df80ae686 100644 --- a/ui/app/routes/jobs/job/definition.js +++ b/ui/app/routes/jobs/job/definition.js @@ -5,33 +5,67 @@ // @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 { + const versionResponse = await job.getVersions(); + const versions = versionResponse.Versions; + definition = versions.findBy('Version', version); + if (!definition) { + throw new Error('Version not found'); + } + } 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(); + } + + console.log({ definition }); 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; format = specificationResponse?.Format ?? 'json'; + console.log({ specification, variableFlags, variableLiteral, format }); } catch (e) { // Swallow the error because Nomad job pre-1.6 will not have a specification } 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..71381a26945 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -322,6 +322,167 @@ 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]`); + console.log('namespace', namespace); + + 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..f8efbb26860 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -501,6 +501,8 @@ module('Unit | Adapter | Job', function (hooks) { assert.equal(request.method, 'GET'); }); + // TODO: test that version requests work for fetchRawDefinition + test('forcePeriodic requests include the activeRegion', async function (assert) { const region = 'region-2'; const job = await this.initializeWithJob({ region }); @@ -681,6 +683,25 @@ 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(), + }; + + // 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'); + }); }); });