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: Edit a job #4612

Merged
merged 18 commits into from
Aug 28, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
51 changes: 27 additions & 24 deletions ui/app/adapters/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,32 +73,35 @@ export default RESTAdapter.extend({
//
// This is the original implementation of _buildURL
// without the pluralization of modelName
urlForFindRecord(id, modelName) {
let path;
let url = [];
let host = get(this, 'host');
let prefix = this.urlPrefix();

if (modelName) {
path = modelName.camelize();
if (path) {
url.push(path);
}
}
urlForFindRecord: urlForRecord,
urlForUpdateRecord: urlForRecord,
});

if (id) {
url.push(encodeURIComponent(id));
}
function urlForRecord(id, modelName) {
let path;
let url = [];
let host = get(this, 'host');
let prefix = this.urlPrefix();

if (prefix) {
url.unshift(prefix);
if (modelName) {
path = modelName.camelize();
if (path) {
url.push(path);
}
}

url = url.join('/');
if (!host && url && url.charAt(0) !== '/') {
url = '/' + url;
}
if (id) {
url.push(encodeURIComponent(id));
}

return url;
},
});
if (prefix) {
url.unshift(prefix);
}

url = url.join('/');
if (!host && url && url.charAt(0) !== '/') {
url = '/' + url;
}

return url;
}
24 changes: 21 additions & 3 deletions ui/app/adapters/job.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ export default Watchable.extend({
return associateNamespace(url, namespace);
},

urlForUpdateRecord(id, type, hash) {
const [name, namespace] = JSON.parse(id);
let url = this._super(name, type, hash);
return associateNamespace(url, namespace);
},

xhrKey(url, method, options = {}) {
const plainKey = this._super(...arguments);
const namespace = options.data && options.data.namespace;
Expand Down Expand Up @@ -71,15 +77,19 @@ export default Watchable.extend({
},

plan(job) {
const url = addToPath(this.urlForFindRecord(job.get('id'), 'job'), '/plan');
const jobId = job.get('id');
const store = this.get('store');
const url = addToPath(this.urlForFindRecord(jobId, 'job'), '/plan');

return this.ajax(url, 'POST', {
data: {
Job: job.get('_newDefinitionJSON'),
Diff: true,
},
}).then(json => {
json.ID = job.get('id');
this.get('store').pushPayload('job-plan', { jobPlans: [json] });
json.ID = jobId;
store.pushPayload('job-plan', { jobPlans: [json] });
return store.peekRecord('job-plan', jobId);
});
},

Expand All @@ -92,6 +102,14 @@ export default Watchable.extend({
},
});
},

update(job) {
return this.ajax(this.urlForUpdateRecord(job.get('id'), 'job'), 'POST', {
data: {
Job: job.get('_newDefinitionJSON'),
},
});
},
});

function associateNamespace(url, namespace) {
Expand Down
102 changes: 102 additions & 0 deletions ui/app/components/job-editor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import Component from '@ember/component';
import { assert } from '@ember/debug';
import { inject as service } from '@ember/service';
import { computed } from '@ember/object';
import { task } from 'ember-concurrency';
import messageFromAdapterError from 'nomad-ui/utils/message-from-adapter-error';
import localStorageProperty from 'nomad-ui/utils/properties/local-storage';

export default Component.extend({
store: service(),
config: service(),

'data-test-job-editor': true,

job: null,
onSubmit() {},
context: computed({
get() {
return this.get('_context');
},
set(key, value) {
const allowedValues = ['new', 'edit'];

assert(`context must be one of: ${allowedValues.join(', ')}`, allowedValues.includes(value));

Choose a reason for hiding this comment

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

My kingdom for static type checking.

This took me a minute to understand. If you find yourself doing this a lot, it might reduce the local noise to extract to a computed util (like the excellent localStorageProperty).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree. Something like whitelistProperty(val1, val2, val3). I'm honestly not happy at all with this component. I didn't want to spend too much time thinking about how best to refactor it to not have this concept of "contexts", so I just went with it.


this.set('_context', value);
return value;
},
}),

_context: null,
parseError: null,
planError: null,
runError: null,

planOutput: null,

showPlanMessage: localStorageProperty('nomadMessageJobPlan', true),
showEditorMessage: localStorageProperty('nomadMessageJobEditor', true),

stage: computed('planOutput', function() {
return this.get('planOutput') ? 'plan' : 'editor';
}),

plan: task(function*() {
this.reset();

try {
yield this.get('job').parse();
} catch (err) {
const error = messageFromAdapterError(err) || 'Could not parse input';
this.set('parseError', error);
this.scrollToError();
return;
}

try {
const plan = yield this.get('job').plan();
this.set('planOutput', plan);
} catch (err) {
const error = messageFromAdapterError(err) || 'Could not plan job';
this.set('planError', error);
this.scrollToError();
}
}).drop(),

submit: task(function*() {
try {
if (this.get('context') === 'new') {
yield this.get('job').run();
} else {
yield this.get('job').update();
}

const id = this.get('job.plainId');
const namespace = this.get('job.namespace.name') || 'default';

this.reset();

// Treat the job as ephemeral and only provide ID parts.
this.get('onSubmit')(id, namespace);
} catch (err) {
const error = messageFromAdapterError(err) || 'Could not submit job';
this.set('runError', error);
this.set('planOutput', null);
this.scrollToError();
}
}),

reset() {
this.set('planOutput', null);
this.set('planError', null);
this.set('parseError', null);
this.set('runError', null);
},

scrollToError() {
if (!this.get('config.isTest')) {
window.scrollTo(0, 0);
}
},
});
18 changes: 18 additions & 0 deletions ui/app/controllers/jobs/job/definition.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,22 @@ import { alias } from '@ember/object/computed';

export default Controller.extend(WithNamespaceResetting, {
job: alias('model.job'),
definition: alias('model.definition'),

isEditing: false,

edit() {
this.get('job').set('_newDefinition', JSON.stringify(this.get('definition'), null, 2));
this.set('isEditing', true);
},

onCancel() {
this.set('isEditing', false);
},

onSubmit(id, namespace) {
this.transitionToRoute('jobs.job', id, {
queryParams: { jobNamespace: namespace },
});

Choose a reason for hiding this comment

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

This might be a stupid question, but bear with me. I think these methods are used as actions in the template, so why aren't they part of an actions object in the controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm experimenting with the usefulness of the actions object. Trying to see if I can get away with only using the actions object when I actually have to, which I imagine is rarely if not never.

I'd like to hear your thoughts on the experiment!

},
});
68 changes: 4 additions & 64 deletions ui/app/controllers/jobs/run.js
Original file line number Diff line number Diff line change
@@ -1,69 +1,9 @@
import Controller from '@ember/controller';
import { inject as service } from '@ember/service';
import { computed } from '@ember/object';
import { task } from 'ember-concurrency';
import messageFromAdapterError from 'nomad-ui/utils/message-from-adapter-error';
import localStorageProperty from 'nomad-ui/utils/properties/local-storage';

export default Controller.extend({
store: service(),

parseError: null,
planError: null,
runError: null,

planOutput: null,

showPlanMessage: localStorageProperty('nomadMessageJobPlan', true),
showEditorMessage: localStorageProperty('nomadMessageJobEditor', true),

stage: computed('planOutput', function() {
return this.get('planOutput') ? 'plan' : 'editor';
}),

plan: task(function*() {
this.reset();

try {
yield this.get('model').parse();
} catch (err) {
const error = messageFromAdapterError(err) || 'Could not parse input';
this.set('parseError', error);
return;
}

try {
yield this.get('model').plan();
const plan = this.get('store').peekRecord('job-plan', this.get('model.id'));
this.set('planOutput', plan);
} catch (err) {
const error = messageFromAdapterError(err) || 'Could not plan job';
this.set('planError', error);
}
}).drop(),

submit: task(function*() {
try {
yield this.get('model').run();

const id = this.get('model.plainId');
const namespace = this.get('model.namespace.name') || 'default';

this.reset();

// navigate to the new job page
this.transitionToRoute('jobs.job', id, {
queryParams: { jobNamespace: namespace },
});
} catch (err) {
const error = messageFromAdapterError(err) || 'Could not submit job';
this.set('runError', error);
}
}),

reset() {
this.set('planOutput', null);
this.set('planError', null);
this.set('parseError', null);
onSubmit(id, namespace) {
this.transitionToRoute('jobs.job', id, {
queryParams: { jobNamespace: namespace },
});
},
});
20 changes: 17 additions & 3 deletions ui/app/models/job.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,11 @@ export default Model.extend({
return this.store.adapterFor('job').run(this);
},

update() {
assert('A job must be parsed before updated', this.get('_newDefinitionJSON'));
return this.store.adapterFor('job').update(this);
},

parse() {
const definition = this.get('_newDefinition');
let promise;
Expand All @@ -211,7 +216,12 @@ export default Model.extend({
// If the definition is already JSON then it doesn't need to be parsed.
const json = JSON.parse(definition);
this.set('_newDefinitionJSON', json);
this.setIDByPayload(json);

// You can't set the ID of a record that already exists
if (this.get('isNew')) {
this.setIdByPayload(json);
}

promise = RSVP.resolve(definition);
} catch (err) {
// If the definition is invalid JSON, assume it is HCL. If it is invalid
Expand All @@ -221,14 +231,14 @@ export default Model.extend({
.parse(this.get('_newDefinition'))
.then(response => {
this.set('_newDefinitionJSON', response);
this.setIDByPayload(response);
this.setIdByPayload(response);
});
}

return promise;
},

setIDByPayload(payload) {
setIdByPayload(payload) {
const namespace = payload.Namespace || 'default';
const id = payload.Name;

Expand All @@ -241,6 +251,10 @@ export default Model.extend({
}
},

resetId() {
this.set('id', JSON.stringify([this.get('plainId'), this.get('namespace.name') || 'default']));
},

statusClass: computed('status', function() {
const classMap = {
pending: 'is-pending',
Expand Down
1 change: 1 addition & 0 deletions ui/app/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Router.map(function() {
this.route('deployments');
this.route('evaluations');
this.route('allocations');
this.route('edit');
});
});

Expand Down
9 changes: 9 additions & 0 deletions ui/app/routes/jobs/job/definition.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,13 @@ export default Route.extend({
definition,
}));
},

resetController(controller, isExiting) {
if (isExiting) {
const job = controller.get('job');
job.rollbackAttributes();
job.resetId();
controller.set('isEditing', false);
}
},
});
2 changes: 1 addition & 1 deletion ui/app/services/watch-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ export default Service.extend({
},

setIndexFor(url, value) {
list[url] = value;
list[url] = +value;
},
});
Loading