From 87bf374747026d783991d3deb4e3e945d4b90dec Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Thu, 10 Oct 2024 14:45:56 -0400 Subject: [PATCH 01/11] Edit from Version functionality --- .changelog/24168.txt | 3 + ui/app/adapters/job.js | 4 +- ui/app/components/job-version.js | 27 +++++++++ ui/app/models/job.js | 4 +- ui/app/routes/jobs/job/definition.js | 37 ++++++++++-- ui/app/templates/components/job-version.hbs | 63 ++++++++++++++++----- ui/tests/unit/adapters/job-test.js | 2 + 7 files changed, 118 insertions(+), 22 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..745aec041e5 100644 --- a/ui/app/adapters/job.js +++ b/ui/app/adapters/job.js @@ -25,11 +25,11 @@ export default class JobAdapter extends WatchableNamespaceIDs { return this.ajax(url, 'GET'); } - fetchRawSpecification(job) { + 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..bcb7228b0b8 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 revertButtonStatus = 'idle'; + @task(function* () { try { const versionBeforeReversion = this.version.get('job.version'); @@ -108,6 +115,26 @@ export default class JobVersion extends Component { }) revertTo; + @action async editFromVersion() { + 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 handleKeydown(event) { if (event.key === 'Escape') { 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..bd670a9d676 100644 --- a/ui/app/routes/jobs/job/definition.js +++ b/ui/app/routes/jobs/job/definition.js @@ -5,29 +5,58 @@ // @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; // query parameter is a string; convert to number const job = this.modelFor('jobs.job'); if (!job) return; - const definition = await job.fetchRawDefinition(); + let definition; + + if (version) { + 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(); + } 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/templates/components/job-version.hbs b/ui/app/templates/components/job-version.hbs index 587bb18c978..c56503db4a1 100644 --- a/ui/app/templates/components/job-version.hbs +++ b/ui/app/templates/components/job-version.hbs @@ -81,20 +81,55 @@
{{#unless this.isCurrent}} {{#if (can "run job" namespace=this.version.job.namespace)}} - + {{#if (eq this.revertButtonStatus 'idle')}} + + {{else if (eq this.revertButtonStatus 'confirming')}} + + Are you sure you want to revert to this version? + + + + + {{else if (eq this.revertButtonStatus 'running')}} + + {{/if}} {{else}} Date: Thu, 10 Oct 2024 16:13:39 -0400 Subject: [PATCH 02/11] Reworked as Clone and Revert --- ui/app/components/job-version.js | 27 ++++++++- ui/app/routes/jobs/job/definition.js | 1 + ui/app/routes/jobs/run/index.js | 11 +++- ui/app/templates/components/job-version.hbs | 63 ++++++++++++--------- 4 files changed, 71 insertions(+), 31 deletions(-) diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index bcb7228b0b8..d777cea1cbe 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -85,7 +85,7 @@ export default class JobVersion extends Component { /** * @type {'idle' | 'confirming'} */ - @tracked revertButtonStatus = 'idle'; + @tracked cloneButtonStatus = 'idle'; @task(function* () { try { @@ -95,6 +95,7 @@ export default class JobVersion extends Component { const versionAfterReversion = this.version.get('job.version'); if (versionBeforeReversion === versionAfterReversion) { + // TODO: I don't think this is ever hit, we have template checks against it. this.args.handleError({ level: 'warn', title: 'Reversion Had No Effect', @@ -115,7 +116,7 @@ export default class JobVersion extends Component { }) revertTo; - @action async editFromVersion() { + @action async cloneAsNewVersion() { try { this.router.transitionTo( 'jobs.job.definition', @@ -135,6 +136,28 @@ export default class JobVersion extends Component { } } + @action async cloneAsNewJob() { + console.log('cloneAsNewJob'); + try { + // TODO: copy the job definition over there. + console.log('Do I have submission info???', this.version); + let job = await this.version.get('job'); + let specification = await job.fetchRawSpecification(this.version.number); + console.log('Do I have specification???', specification); + let specificationSourceString = specification.Source; // TODO: should do some Format checking here, at the very least. + this.router.transitionTo('jobs.run', { + queryParams: { + sourceString: specificationSourceString, + }, + }); + } catch (e) { + this.args.handleError({ + level: 'danger', + title: 'Could Not Clone as New Job', + }); + } + } + @action handleKeydown(event) { if (event.key === 'Escape') { diff --git a/ui/app/routes/jobs/job/definition.js b/ui/app/routes/jobs/job/definition.js index bd670a9d676..98277cbda2d 100644 --- a/ui/app/routes/jobs/job/definition.js +++ b/ui/app/routes/jobs/job/definition.js @@ -26,6 +26,7 @@ export default class DefinitionRoute extends Route { */ async model({ version }) { version = +version; // query parameter is a string; convert to number + /** @type {import('../../../models/job').default} */ const job = this.modelFor('jobs.job'); if (!job) return; diff --git a/ui/app/routes/jobs/run/index.js b/ui/app/routes/jobs/run/index.js index 95c0bbffd2d..ff48debc6bd 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,12 @@ export default class JobsRunIndexRoute extends Route { return this.store.createRecord('job', { _newDefinition: templateRecord.items.template, }); + } else if (sourceString) { + console.log('elsif', sourceString); + // Add an alert to the page to let the user know that they are submitting a job from a template, and that they should change the name! + return this.store.createRecord('job', { + _newDefinition: sourceString, + }); } else { return this.store.createRecord('job'); } diff --git a/ui/app/templates/components/job-version.hbs b/ui/app/templates/components/job-version.hbs index c56503db4a1..789e6649335 100644 --- a/ui/app/templates/components/job-version.hbs +++ b/ui/app/templates/components/job-version.hbs @@ -81,53 +81,60 @@
{{#unless this.isCurrent}} {{#if (can "run job" namespace=this.version.job.namespace)}} - {{#if (eq this.revertButtonStatus 'idle')}} + {{#if (eq this.cloneButtonStatus 'idle')}} - {{else if (eq this.revertButtonStatus 'confirming')}} - + + + + + {{else if (eq this.cloneButtonStatus 'confirming')}} + {{!-- Are you sure you want to revert to this version? - + --}} - {{else if (eq this.revertButtonStatus 'running')}} - {{/if}} {{else}} From 65b93ce6fac6c17addc584ceafda56edfde07209 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Mon, 21 Oct 2024 17:03:15 -0400 Subject: [PATCH 03/11] Change name warning for cloning as new job, version 0 checking, and erroring on sourceless clone --- ui/app/components/job-version.js | 7 ++----- ui/app/routes/jobs/job/definition.js | 4 +++- ui/app/routes/jobs/run/index.js | 3 ++- ui/app/templates/jobs/run/index.hbs | 8 +++++++- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index d777cea1cbe..b32b7927c49 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -95,7 +95,6 @@ export default class JobVersion extends Component { const versionAfterReversion = this.version.get('job.version'); if (versionBeforeReversion === versionAfterReversion) { - // TODO: I don't think this is ever hit, we have template checks against it. this.args.handleError({ level: 'warn', title: 'Reversion Had No Effect', @@ -139,12 +138,9 @@ export default class JobVersion extends Component { @action async cloneAsNewJob() { console.log('cloneAsNewJob'); try { - // TODO: copy the job definition over there. - console.log('Do I have submission info???', this.version); let job = await this.version.get('job'); let specification = await job.fetchRawSpecification(this.version.number); - console.log('Do I have specification???', specification); - let specificationSourceString = specification.Source; // TODO: should do some Format checking here, at the very least. + let specificationSourceString = specification.Source; this.router.transitionTo('jobs.run', { queryParams: { sourceString: specificationSourceString, @@ -154,6 +150,7 @@ export default class JobVersion extends Component { this.args.handleError({ level: 'danger', title: 'Could Not Clone as New Job', + description: messageForError(e), }); } } diff --git a/ui/app/routes/jobs/job/definition.js b/ui/app/routes/jobs/job/definition.js index 98277cbda2d..aead71582c9 100644 --- a/ui/app/routes/jobs/job/definition.js +++ b/ui/app/routes/jobs/job/definition.js @@ -31,8 +31,10 @@ export default class DefinitionRoute extends Route { if (!job) return; let definition; + console.log('version', version); - if (version) { + if (version !== undefined) { + // version can be 0, can't !version check. try { const versionResponse = await job.getVersions(); const versions = versionResponse.Versions; diff --git a/ui/app/routes/jobs/run/index.js b/ui/app/routes/jobs/run/index.js index ff48debc6bd..5f81cee76a6 100644 --- a/ui/app/routes/jobs/run/index.js +++ b/ui/app/routes/jobs/run/index.js @@ -49,7 +49,7 @@ export default class JobsRunIndexRoute extends Route { _newDefinition: templateRecord.items.template, }); } else if (sourceString) { - console.log('elsif', sourceString); + // console.log('elsif', sourceString); // Add an alert to the page to let the user know that they are submitting a job from a template, and that they should change the name! return this.store.createRecord('job', { _newDefinition: sourceString, @@ -81,6 +81,7 @@ export default class JobsRunIndexRoute extends Route { if (isExiting) { controller.model?.deleteRecord(); controller.set('template', null); + controller.set('sourceString', null); } } } diff --git a/ui/app/templates/jobs/run/index.hbs b/ui/app/templates/jobs/run/index.hbs index 4292cfe5873..56beae01aec 100644 --- a/ui/app/templates/jobs/run/index.hbs +++ b/ui/app/templates/jobs/run/index.hbs @@ -6,5 +6,11 @@ {{page-title "Run a job"}}
+ {{#if this.sourceString}} + + 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 + From 606133ac91c3930281da05fe63c553494942adef Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Wed, 23 Oct 2024 10:46:34 -0400 Subject: [PATCH 04/11] 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 --- ui/app/controllers/jobs/run/index.js | 2 +- ui/app/routes/jobs/job/definition.js | 2 +- ui/app/routes/jobs/run/index.js | 1 + ui/app/templates/components/job-editor/alert.hbs | 4 +++- ui/app/templates/jobs/run/index.hbs | 2 +- 5 files changed, 7 insertions(+), 4 deletions(-) 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/routes/jobs/job/definition.js b/ui/app/routes/jobs/job/definition.js index aead71582c9..6df6d2fb6dc 100644 --- a/ui/app/routes/jobs/job/definition.js +++ b/ui/app/routes/jobs/job/definition.js @@ -25,7 +25,7 @@ export default class DefinitionRoute extends Route { * specification, variableFlags, and variableLiteral. */ async model({ version }) { - version = +version; // query parameter is a string; convert to number + 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; diff --git a/ui/app/routes/jobs/run/index.js b/ui/app/routes/jobs/run/index.js index 5f81cee76a6..8774a86f2a9 100644 --- a/ui/app/routes/jobs/run/index.js +++ b/ui/app/routes/jobs/run/index.js @@ -82,6 +82,7 @@ export default class JobsRunIndexRoute extends Route { 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/jobs/run/index.hbs b/ui/app/templates/jobs/run/index.hbs index 56beae01aec..3532ed3a1c1 100644 --- a/ui/app/templates/jobs/run/index.hbs +++ b/ui/app/templates/jobs/run/index.hbs @@ -6,7 +6,7 @@ {{page-title "Run a job"}}
- {{#if this.sourceString}} + {{#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. From 37a125bdaaa43eedfe0526414706c559977026bd Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Thu, 7 Nov 2024 16:27:01 -0500 Subject: [PATCH 05/11] A few code comments and log cleanup --- ui/app/adapters/job.js | 14 ++++++++++++++ ui/app/routes/jobs/job/definition.js | 6 ++++-- ui/app/routes/jobs/run/index.js | 2 -- ui/app/templates/components/job-version.hbs | 4 ---- 4 files changed, 18 insertions(+), 8 deletions(-) diff --git a/ui/app/adapters/job.js b/ui/app/adapters/job.js index 745aec041e5..384419b533e 100644 --- a/ui/app/adapters/job.js +++ b/ui/app/adapters/job.js @@ -20,11 +20,25 @@ 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'); } + /** + * 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'), diff --git a/ui/app/routes/jobs/job/definition.js b/ui/app/routes/jobs/job/definition.js index 6df6d2fb6dc..84df80ae686 100644 --- a/ui/app/routes/jobs/job/definition.js +++ b/ui/app/routes/jobs/job/definition.js @@ -31,10 +31,9 @@ export default class DefinitionRoute extends Route { if (!job) return; let definition; - console.log('version', version); if (version !== undefined) { - // version can be 0, can't !version check. + // Not (!version) because version can be 0 try { const versionResponse = await job.getVersions(); const versions = versionResponse.Versions; @@ -54,6 +53,8 @@ export default class DefinitionRoute extends Route { definition = await job.fetchRawDefinition(); } + console.log({ definition }); + let format = 'json'; // default to json in network request errors let specification; let variableFlags; @@ -64,6 +65,7 @@ export default class DefinitionRoute extends Route { 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 8774a86f2a9..38e11617335 100644 --- a/ui/app/routes/jobs/run/index.js +++ b/ui/app/routes/jobs/run/index.js @@ -49,8 +49,6 @@ export default class JobsRunIndexRoute extends Route { _newDefinition: templateRecord.items.template, }); } else if (sourceString) { - // console.log('elsif', sourceString); - // Add an alert to the page to let the user know that they are submitting a job from a template, and that they should change the name! return this.store.createRecord('job', { _newDefinition: sourceString, }); diff --git a/ui/app/templates/components/job-version.hbs b/ui/app/templates/components/job-version.hbs index 789e6649335..9a5a160e671 100644 --- a/ui/app/templates/components/job-version.hbs +++ b/ui/app/templates/components/job-version.hbs @@ -88,7 +88,6 @@ @color="secondary" @size="small" @isInline={{true}} - {{!-- @disabled={{this.revertTo.isRunning}} --}} {{on "click" (action (mut this.cloneButtonStatus) "confirming")}} /> @@ -109,9 +108,6 @@ {{else if (eq this.cloneButtonStatus 'confirming')}} - {{!-- - Are you sure you want to revert to this version? - --}} Date: Thu, 7 Nov 2024 16:58:07 -0500 Subject: [PATCH 06/11] Scaffolding new acceptance tests --- ui/tests/acceptance/job-versions-test.js | 39 ++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index b16ac6eda88..940d15a5c27 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -322,6 +322,45 @@ 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'); + + const managementToken = server.create('token'); + window.localStorage.nomadTokenSecret = managementToken.secretId; + + job = server.create('job', { createAllocations: false, version: 99 }); + // remove auto-created versions and create 3 of them, one with a tag + server.db.jobVersions.remove(); + server.create('job-version', { job, version: 99 }); + server.create('job-version', { + job, + version: 98, + versionTag: { + Name: 'test-tag', + Description: 'A tag with a brief description', + }, + }); + server.create('job-version', { job, version: 97 }); + await Versions.visit({ id: job.id }); + }); + + test('You can clone a version as a new job', async function (assert) { + // Buttons exist + 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' + ); + }); +}); + module('Acceptance | job versions (with client token)', function (hooks) { setupApplicationTest(hooks); setupMirage(hooks); From 5bbc9ee67b9a40fca197a132c2b806ecb9c2a4d9 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 8 Nov 2024 16:55:28 -0500 Subject: [PATCH 07/11] A whack of fun new tests --- ui/app/templates/components/job-version.hbs | 33 +++-- ui/app/templates/jobs/run/index.hbs | 2 +- ui/tests/acceptance/job-versions-test.js | 132 +++++++++++++++++++- 3 files changed, 144 insertions(+), 23 deletions(-) diff --git a/ui/app/templates/components/job-version.hbs b/ui/app/templates/components/job-version.hbs index 9a5a160e671..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}} @@ -91,22 +91,21 @@ {{on "click" (action (mut this.cloneButtonStatus) "confirming")}} /> - - - + {{else if (eq this.cloneButtonStatus 'confirming')}} {{#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. diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index 940d15a5c27..71381a26945 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -329,28 +329,41 @@ module('Acceptance | job versions (clone and edit)', function (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 }); + 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 }); + 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: 97 }); + server.create('job-version', { + job, + version: 0, + submitTime: 1731101585761339000, + }); await Versions.visit({ id: job.id }); }); - test('You can clone a version as a new job', async function (assert) { - // Buttons exist + test('Clone and edit buttons are shown', async function (assert) { assert.dom('.job-version').exists({ count: 3 }); assert .dom('[data-test-clone-and-edit]') @@ -358,6 +371,115 @@ module('Acceptance | job versions (clone and edit)', function (hooks) { { 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(); }); }); From 629489047356adbd5344edcef139889459225730 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Sun, 10 Nov 2024 09:37:31 -0500 Subject: [PATCH 08/11] Unit test for version number url passing on fetchRawDef --- ui/tests/unit/adapters/job-test.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/ui/tests/unit/adapters/job-test.js b/ui/tests/unit/adapters/job-test.js index 21fad651fc9..f8efbb26860 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -683,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'); + }); }); }); From 18b2bd31284707b569bf9c548602d575fcbc52a8 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Sun, 24 Nov 2024 09:36:24 -0500 Subject: [PATCH 09/11] Bit of cleanup --- ui/app/components/job-version.js | 1 - ui/app/routes/jobs/job/definition.js | 3 --- ui/tests/acceptance/job-versions-test.js | 1 - 3 files changed, 5 deletions(-) diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index b32b7927c49..a97e8e12ebe 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -136,7 +136,6 @@ export default class JobVersion extends Component { } @action async cloneAsNewJob() { - console.log('cloneAsNewJob'); try { let job = await this.version.get('job'); let specification = await job.fetchRawSpecification(this.version.number); diff --git a/ui/app/routes/jobs/job/definition.js b/ui/app/routes/jobs/job/definition.js index 84df80ae686..12a4a54c7f8 100644 --- a/ui/app/routes/jobs/job/definition.js +++ b/ui/app/routes/jobs/job/definition.js @@ -53,8 +53,6 @@ export default class DefinitionRoute extends Route { definition = await job.fetchRawDefinition(); } - console.log({ definition }); - let format = 'json'; // default to json in network request errors let specification; let variableFlags; @@ -65,7 +63,6 @@ export default class DefinitionRoute extends Route { 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/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index 71381a26945..56b4ad623cd 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -420,7 +420,6 @@ module('Acceptance | job versions (clone and edit)', function (hooks) { 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(), From 34a566ada08ccb28d1756daf2a20d6ab8198a5e6 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Mon, 25 Nov 2024 13:28:34 -0500 Subject: [PATCH 10/11] fetchRawDefinition gets version support at adapter layer --- ui/app/adapters/job.js | 24 +++++++++-- ui/app/models/job.js | 4 +- ui/app/routes/jobs/job/definition.js | 7 +--- ui/tests/unit/adapters/job-test.js | 61 +++++++++++++++++++++++++++- 4 files changed, 84 insertions(+), 12 deletions(-) diff --git a/ui/app/adapters/job.js b/ui/app/adapters/job.js index 384419b533e..b781bf8008c 100644 --- a/ui/app/adapters/job.js +++ b/ui/app/adapters/job.js @@ -27,10 +27,28 @@ export default class JobAdapter extends WatchableNamespaceIDs { * 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 */ - fetchRawDefinition(job) { - const url = this.urlForFindRecord(job.get('id'), 'job'); - return this.ajax(url, 'GET'); + 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; } /** diff --git a/ui/app/models/job.js b/ui/app/models/job.js index cca22f8fcaa..007c8a699b5 100644 --- a/ui/app/models/job.js +++ b/ui/app/models/job.js @@ -537,8 +537,8 @@ 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(version) { diff --git a/ui/app/routes/jobs/job/definition.js b/ui/app/routes/jobs/job/definition.js index 12a4a54c7f8..31dabec20ae 100644 --- a/ui/app/routes/jobs/job/definition.js +++ b/ui/app/routes/jobs/job/definition.js @@ -35,12 +35,7 @@ export default class DefinitionRoute extends Route { 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'); - } + definition = await job.fetchRawDefinition(version); } catch (e) { console.error('error fetching job version definition', e); this.notifications.add({ diff --git a/ui/tests/unit/adapters/job-test.js b/ui/tests/unit/adapters/job-test.js index f8efbb26860..370573113c7 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -501,7 +501,64 @@ module('Unit | Adapter | Job', function (hooks) { assert.equal(request.method, 'GET'); }); - // TODO: test that version requests work for fetchRawDefinition + 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'; @@ -688,6 +745,8 @@ module('Unit | Adapter | Job', function (hooks) { 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({})); From de844b3c0b61b07412201bab16aeda7a13c5db20 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Mon, 25 Nov 2024 15:45:58 -0500 Subject: [PATCH 11/11] Handle spec-not-available-but-definition-is for clone-as-new-job --- ui/app/components/job-version.js | 33 ++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index a97e8e12ebe..803491011ea 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -136,21 +136,34 @@ export default class JobVersion extends Component { } @action async cloneAsNewJob() { + const job = await this.version.get('job'); try { - let job = await this.version.get('job'); - let specification = await job.fetchRawSpecification(this.version.number); - let specificationSourceString = specification.Source; + const specification = await job.fetchRawSpecification( + this.version.number + ); this.router.transitionTo('jobs.run', { queryParams: { - sourceString: specificationSourceString, + sourceString: specification.Source, }, }); - } catch (e) { - this.args.handleError({ - level: 'danger', - title: 'Could Not Clone as New Job', - description: messageForError(e), - }); + 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), + }); + } } }