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

Conversation

philrenaud
Copy link
Contributor

@philrenaud philrenaud commented Oct 10, 2024

Adds "Clone and Edit" alongside "Revert Version" on the Versions page:

There are two functional paths here:

  • Cloning as a new version of the job
  • Cloning as a new job altogether

In each of these cases, we also have a split on "fetchRawDefinition" (fetches the submitted HCL or JSON jobspec) or "fetchRawSpecification" (the pre-1.5 Nomad way of getting job details, which the UI falls back to if submission/definition is not available). A lot of the duplicate-looking code in this PR is to address both paths.

Similarly, there are some safeguards included in this PR for things like ensuring users who clone to a new job change the job's name, lest it adds it as a new version unintentionally.

image

Because we're passing template strings via query params, this means you can use ?sourceString= on the jobs run page to send someone a readily-editable job spec, using a URL like this.

This also adds a few UX conveniences like prompting a user to "run as a new job instead" if they try updating a job definition and changing its name (which fails a plan request), and reminding them to change the job name in the event they try to clone as a new one
image

Resolves #24156

Copy link

github-actions bot commented Oct 10, 2024

Ember Test Audit comparison

main af31960 change
passes 1581 1581 0
failures 0 0 0
flaky 0 0 0
duration 13m 38s 594ms 13m 27s 414ms -11s 180ms

@aimeeu
Copy link
Contributor

aimeeu commented Oct 10, 2024

@philrenaud when this is merged, we also need to update the screenshots, and possibly text, in the job versions guide.

@philrenaud philrenaud marked this pull request as ready for review November 10, 2024 14:41
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

}

@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');

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?

Comment on lines +9 to +14
{{#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}}
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.

@@ -501,6 +501,8 @@ module('Unit | Adapter | Job', function (hooks) {
assert.equal(request.method, 'GET');
});

// TODO: test that version requests work for fetchRawDefinition
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to resolve this TODO in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.9.x backport to 1.9.x release line hcc/jira
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI - Versions - Revert Version improvements
3 participants