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.

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] "Clone and Edit" functionality for versions #24168

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .changelog/24168.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
ui: Add an Edit From Version button as an option when reverting from an older job version
```
18 changes: 16 additions & 2 deletions ui/app/adapters/job.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand Down
47 changes: 47 additions & 0 deletions ui/app/components/job-version.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

Debris from debugging, I think?

Suggested change
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') {
Expand Down
2 changes: 1 addition & 1 deletion ui/app/controllers/jobs/run/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
4 changes: 2 additions & 2 deletions ui/app/models/job.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
42 changes: 38 additions & 4 deletions ui/app/routes/jobs/job/definition.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object>} 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 });
Copy link
Member

Choose a reason for hiding this comment

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

More debugging?

} catch (e) {
// Swallow the error because Nomad job pre-1.6 will not have a specification
}
Expand Down
11 changes: 10 additions & 1 deletion ui/app/routes/jobs/run/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ export default class JobsRunIndexRoute extends Route {
template: {
refreshModel: true,
},
sourceString: {
refreshModel: true,
},
};

beforeModel(transition) {
Expand All @@ -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.
Expand All @@ -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');
}
Expand Down Expand Up @@ -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);
}
}
}
4 changes: 3 additions & 1 deletion ui/app/templates/components/job-editor/alert.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
<Hds::Alert @type="inline" @color="critical" data-test-error={{@data.error.type}} as |A|>
<A.Title data-test-error-title>{{conditionally-capitalize @data.error.type true}}</A.Title>
<A.Description data-test-error-message>{{@data.error.message}}</A.Description>
{{#if (eq @data.error.message "Job ID does not match")}}
<A.Button @text="Run as a new job instead" @color="primary" @route="jobs.run" @query={{hash [email protected]._newDefinition disregardNameWarning=true}} />
{{/if}}
</Hds::Alert>
{{/if}}
{{#if (and (eq @data.stage "read") @data.hasVariables (eq @data.view "job-spec"))}}
Expand All @@ -31,4 +34,3 @@
</Hds::Alert>
{{/if}}
</div>

67 changes: 52 additions & 15 deletions ui/app/templates/components/job-version.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
~}}

{{did-update this.versionsDidUpdate this.diff}}
<section class="job-version">
<section class="job-version" data-test-job-version={{this.version.number}}>
<div class="boxed-section {{if this.version.versionTag "tagged"}}" data-test-tagged-version={{if this.version.versionTag "true" "false"}}>
<header class="boxed-section-head is-light inline-definitions">
Version #{{this.version.number}}
Expand Down Expand Up @@ -81,20 +81,57 @@
<div class="version-options">
{{#unless this.isCurrent}}
{{#if (can "run job" namespace=this.version.job.namespace)}}
<TwoStepButton
data-test-revert-to
@classes={{hash
idleButton="is-warning is-outlined"
confirmButton="is-warning"}}
@fadingBackground={{true}}
@idleText="Revert Version"
@cancelText="Cancel"
@confirmText="Yes, Revert Version"
@confirmationMessage="Are you sure you want to revert to this version?"
@inlineText={{true}}
@size="small"
@awaitingConfirmation={{this.revertTo.isRunning}}
@onConfirm={{perform this.revertTo}} />
{{#if (eq this.cloneButtonStatus 'idle')}}
<Hds::Button
data-test-clone-and-edit
@text="Clone and Edit"
@color="secondary"
@size="small"
@isInline={{true}}
{{on "click" (action (mut this.cloneButtonStatus) "confirming")}}
/>

<TwoStepButton
data-test-revert-to
@classes={{hash
idleButton="is-warning is-outlined"
confirmButton="is-warning"}}
@fadingBackground={{true}}
@idleText="Revert Version"
@cancelText="Cancel"
@confirmText="Yes, Revert Version"
@confirmationMessage="Are you sure you want to revert to this version?"
@inlineText={{true}}
@size="small"
@awaitingConfirmation={{this.revertTo.isRunning}}
@onConfirm={{perform this.revertTo}}
/>
{{else if (eq this.cloneButtonStatus 'confirming')}}
<Hds::Button
data-test-cancel-clone
@text="Cancel"
@color="secondary"
@size="small"
@isInline={{true}}
{{on "click" (action (mut this.cloneButtonStatus) "idle")}}
/>
<Hds::Button
data-test-clone-as-new-version
@text="Clone as New Version of {{this.version.job.name}}"
@color="secondary"
@size="small"
@isInline={{true}}
{{on "click" (action this.cloneAsNewVersion)}}
/>
<Hds::Button
data-test-clone-as-new-job
@text="Clone as New Job"
@color="secondary"
@size="small"
@isInline={{true}}
{{on "click" (action this.cloneAsNewJob)}}
/>
{{/if}}
{{else}}
<Hds::Button
data-test-revert-to
Expand Down
8 changes: 7 additions & 1 deletion ui/app/templates/jobs/run/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,11 @@
<Breadcrumb @crumb={{hash label="Run" args=(array "jobs.run")}} />
{{page-title "Run a job"}}
<section class="section">
{{#if (and this.sourceString (not this.disregardNameWarning))}}
<Hds::Alert @type="inline" @color="warning" data-test-job-name-warning as |A|>
<A.Title>Don't forget to change the job name!</A.Title>
<A.Description>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.</A.Description>
</Hds::Alert>
{{/if}}
Comment on lines +9 to +14
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to swap out the job name with an empty string to prevent accidental overwrites. Unfortunately we don't a JS parser for HCL2. My first pass at a regexp was something like ^job\s+"(.*)"\s+\{ but some quick testing finds some corner cases, so maybe that's not such a good idea after all. If you've got an obvious way to do this, I'd go for it, otherwise we can skip.

<JobEditor @job={{this.model}} @context="new" @onSubmit={{action this.onSubmit}} @handleSaveAsTemplate={{this.handleSaveAsTemplate}} data-test-job-editor />
</section>
</section>
Loading
Loading