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: job templates manager view #15815

Merged
merged 3 commits into from
Jan 20, 2023
Merged
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
17 changes: 17 additions & 0 deletions ui/app/controllers/jobs/run/templates/manage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import Controller from '@ember/controller';
import { tracked } from '@glimmer/tracking';

export default class JobsRunTemplatesController extends Controller {
@tracked selectedTemplate = null;

columns = ['name', 'namespace', 'description'].map((column) => {
return {
key: column,
label: `${column.charAt(0).toUpperCase()}${column.substring(1)}`,
};
});

formatTemplateLabel(path) {
return path.split('nomad/job-templates/')[1];
}
}
8 changes: 8 additions & 0 deletions ui/app/helpers/invoke-fn.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { helper } from '@ember/component/helper';

function invokeFn([scope, fn]) {
let args = arguments[0].slice(2);
return fn.apply(scope, args);
}

export default helper(invokeFn);
31 changes: 31 additions & 0 deletions ui/app/routes/jobs/run/templates/manage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';

export default class RunTemplatesRoute extends Route {
@service can;
@service router;
@service store;

beforeModel() {
const hasPermissions = this.can.can('write variable', null, {
namespace: '*',
path: '*',
});

if (!hasPermissions) {
this.router.transitionTo('jobs');
}
}

async model() {
const jobTemplateVariables = await this.store.query('variable', {
prefix: 'nomad/job-templates',
namespace: '*',
});
const recordsToQuery = jobTemplateVariables.map((template) =>
this.store.findRecord('variable', template.id)
);
Comment on lines +21 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing all variables here in the Mirage mocks:
image - is prefix the correct property, or is path?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: also seeing more variables than expected on the non-management list page (templates index)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In mirage mocks you will see all the variables here because we're using the /vars endpoint which does not have any logic for accounting for the prefix. Touching our Mirage setup would spill over into the variables view so I'm not going to touch it until after all the work is complete and we don't have more feature requests or updates on the feature.

Its annoying but we'll have to rely on a Nomad server here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood! Let's definitely tackle this at some point in the process, if not in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey! Get back to being on vacation and recharge your batteries!!!


return Promise.all(recordsToQuery);
}
}
1 change: 1 addition & 0 deletions ui/app/templates/jobs/run/templates/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
<Hds::Button @text="Cancel" @route="jobs.run" @color="critical" data-test-cancel />
</Hds::ButtonSet>
<Hds::ButtonSet class="button-group align-right">
<Hds::Button @text="Manage" @color="tertiary" @icon="edit" @route="jobs.run.templates.manage" data-test-manage-button />
<Hds::Button @text="Create New Template" @color="tertiary" @icon="file-plus" @route="jobs.run.templates.new" data-test-create-new-button />
</Hds::ButtonSet>
</footer>
Expand Down
34 changes: 34 additions & 0 deletions ui/app/templates/jobs/run/templates/manage.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
{{page-title "Manage templates"}}
<section class="section">
<header class="run-job-header">
<h1 class="title is-3">Choose from a template</h1>
<p>A plan will be requested before the job is submitted. You can pick a template or write your own job spec.</p>
</header>
{{#if (eq this.model.length 0)}}
<h3 data-test-empty-templates-list-headline class="empty-message-headline">
You have no templates to choose from. Would you like to <Hds::Link::Inline @route="jobs.run.templates.new" data-test-create-inline>create</Hds::Link::Inline> one?
</h3>
<Hds::Button class="button-group" @text="Back to editor" @route="jobs.run" @icon="arrow-left" data-test-cancel />
{{else}}
<main class="radio-group" data-test-template-list>
<Hds::Table @model={{this.model}} @columns={{this.columns}}>
<:body as |B|>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we change B to Template or something more indicative of what we're iterating over?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't a template. Rather it's a contextual components that is yielding out TableRow and TableData components. We could change the name to Body to reflect this. And its a pattern used by other teams, in our own codebase and in line with the official documentation.

Changing this to Body could make sense. But that would defeat the purpose of the alias and we could just write :body.Tr instead.

Your call. I'm open to suggestions!

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes a ton of sense. Thanks for clearing it up for me!

<B.Tr>
<B.Td>
<LinkTo @route="jobs.run.templates.template" @model={{concat B.data.path "@" B.data.namespace}} data-test-edit-template={{B.data.path}}>
{{invoke-fn this this.formatTemplateLabel B.data.path}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice and reminds me of svelte attributes!

</LinkTo>
</B.Td>
<B.Td>{{B.data.namespace}}</B.Td>
<B.Td>{{B.data.items.description}}</B.Td>
</B.Tr>
</:body>
</Hds::Table>
</main>
<footer class="buttonset">
<Hds::ButtonSet class="button-group">
<Hds::Button @icon="arrow-left" @text="Done" @route="jobs.run.templates" data-test-done />
</Hds::ButtonSet>
</footer>
{{/if}}
</section>
56 changes: 56 additions & 0 deletions ui/tests/acceptance/job-run-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
currentRouteName,
currentURL,
fillIn,
visit,
} from '@ember/test-helpers';
import { assign } from '@ember/polyfills';
import { module, test } from 'qunit';
Expand Down Expand Up @@ -424,5 +425,60 @@ module('Acceptance | job run', function (hooks) {
'Template is filled out with text from the editor.'
);
});

test('a user can edit a template', async function (assert) {
assert.expect(5);

// Arrange
server.create('variable', {
path: 'nomad/job-templates/foo',
namespace: 'default',
id: 'nomad/job-templates/foo',
});

await visit('/jobs/run/templates/manage');

assert.equal(currentRouteName(), 'jobs.run.templates.manage');
assert
.dom('[data-test-template-list]')
.exists('A list of templates is visible');

await click('[data-test-edit-template="nomad/job-templates/foo"]');
assert.equal(
currentRouteName(),
'jobs.run.templates.template',
'Navigates to edit template view'
);

server.put('/var/:varId', function (_server, fakeRequest) {
assert.deepEqual(
JSON.parse(fakeRequest.requestBody),
{
Path: 'nomad/job-templates/foo',
CreateIndex: null,
ModifyIndex: null,
Namespace: 'default',
ID: 'nomad/job-templates/foo',
Items: { description: 'baz qud thud' },
},
'It makes a PUT request to the /vars/:varId endpoint with the appropriate request body for job templates.'
);

return {
Items: { description: 'baz qud thud' },
Namespace: 'default',
Path: 'nomad/job-templates/foo',
};
});

await fillIn('[data-test-template-description]', 'baz qud thud');
await click('[data-test-edit-template]');

assert.equal(
currentRouteName(),
'jobs.run.templates.index',
'We navigate back to the templates view.'
);
});
});
});