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

UI: Edit a job #4612

merged 18 commits into from
Aug 28, 2018

Conversation

DingoEatingFuzz
Copy link
Contributor

This is a part of the larger Job Writes in the UI project #4600

This is very similar to the Run Job UI, but it's for editing a job. Those of you familiar with Nomad know that editing a job and running a job is nearly the same. This just offers guarantees that you won't accidentally make a new job similar to the one you are editing.

The edit button lives on the Job Definition page, which seems like a good place for it.

Reviewer notes:

Somehow, even though I knew I was going to do this work, I ended up with non-trivial refactor 🤦‍♂️ . I moved what was the Run controller to a new Job Editor component and I moved/rewrote the tests that were once Run Page Acceptance Tests to be Job Editor Integration Tests.

image

image

image

image

The lowest valid blocking query index is 1, but the API will return 0 if
there has yet to be an index set (no response). This in conjunction with
that 0 being stored as a string made the "fallback to 1" guard not work.
Caused by the re-indexing that occurs to remove zero-value bars.
Instead it was using the page-layout is-right div
…eeded

This exports a function that returns the pertinent function when given a
container. This way it works in registerHelper and in integration tests
beforeEach hooks
Most of this was ported over from the existing job run acceptance tests
@DingoEatingFuzz DingoEatingFuzz requested a review from a team August 23, 2018 22:49
This was referenced Aug 23, 2018
Copy link

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍 I initially had some questions around the setIndexFor and distribution bar changes, which were answered by a git log, so thanks for writing good commit messages 😄

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!

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.

@DingoEatingFuzz DingoEatingFuzz merged commit 916cc52 into f-ui-job-writes Aug 28, 2018
@endocrimes endocrimes deleted the f-ui-job-edit branch December 6, 2018 23:35
backspace added a commit that referenced this pull request Jul 4, 2019
This looks to me to be a route-not-taken in #4612.
backspace added a commit that referenced this pull request Jul 23, 2019
This is an extraneous addition from #4612.
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants