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: create new job template #15744

Merged
merged 13 commits into from
Jan 17, 2023
Merged

Conversation

ChaiWithJai
Copy link
Contributor

Closes #15548

timeout: 5000,
});

this.router.transitionTo('jobs.run.templates');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philrenaud there's some funky behavior for how we handle deleting a record on exiting the controller that has a slight conflict with saving. I handled the problem slightly differently than you did with variables by addressing this logic in resetController.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I appreciate the different solution attempted here but I"m running into a breaking error when I try this:

Error: Attempted to handle event deleteRecordon <variable:nomad/job-templates/my fun template@default> while in state root.loaded.created.inFlight.

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 unable to reproduce this. Here's a Replay showing that I'm getting to success.

Could you record a Replay so that I can trace what's happening to start debugging here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look like calling unloadRecord patches our problem here.

@@ -69,7 +69,7 @@ export default class RunRoute extends Route {

resetController(controller, isExiting) {
if (isExiting) {
controller.model.deleteRecord();
controller.model?.deleteRecord();
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 the event that a user has a bad URL, we will throw an error on fetching the template before the job model is created. So we need some type safety here to account for this situation.

@github-actions
Copy link

github-actions bot commented Jan 10, 2023

Ember Asset Size action

As of d4d6825

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +9.72 kB +1.57 kB
nomad-ui.css +358 B +130 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
vendor.css 0 B 0 B

Copy link
Contributor Author

@ChaiWithJai ChaiWithJai left a comment

Choose a reason for hiding this comment

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

@philrenaud This PR can successfully handle the happy path of creating and saving a new variable. However, I need some guidance re: error handling.

  • Happy Path -- Should we have a confirmation message on hitting Cancel or navigating away on creation?
  • Sad Path -- Using duplicate template name?
  • Sad Path -- Form Validation -- bad template name that isn't compliant with RFC3986 URL-safe characters
  • Sad Path -- Form Validation -- no description
  • Sad Path -- Invalid JSON

Copy link
Contributor

@philrenaud philrenaud left a comment

Choose a reason for hiding this comment

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

Code looks pretty good, but smoke testing revealed some bugs.

data-test-template-name
/>
</label>
<SingleSelectDropdown
Copy link
Contributor

Choose a reason for hiding this comment

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

Running into a fatal error here:
image

(I don't have namespaces other than default)

I think you probably want to wrap this in a {{#if this.system.shouldShowNamespaces}} conditional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be patched by some conditional logic here for when namespaces are enabled.

ui/app/templates/jobs/run/templates/new.hbs Show resolved Hide resolved
timeout: 5000,
});

this.router.transitionTo('jobs.run.templates');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I appreciate the different solution attempted here but I"m running into a breaking error when I try this:

Error: Attempted to handle event deleteRecordon <variable:nomad/job-templates/my fun template@default> while in state root.loaded.created.inFlight.

ui/app/templates/jobs/run/templates/new.hbs Outdated Show resolved Hide resolved
Comment on lines 4 to 16
<label>
<span>
Template name
</span>
<Input
@type="text"
@value={{mut this.templateName}}
placeholder="your-template-name-here"
class="input path-input"
{{autofocus}}
data-test-template-name
/>
</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want this here; we should instead add it into the VariableForm::JobTemplateEditor. Name is already a field there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! I'm following the pattern in variable-form.hbs, where path isn't included in the variable-form.job-template-editor component.

Do you want me to create this as a conditionally rendered part of the template in that component?

ui/app/controllers/jobs/run/templates/new.js Show resolved Hide resolved
…5767)

* ui: handle server side errors

* ui: show error to prevent duplicate

* refact: conditional namespace

* ui: save as template flow (#15787)
@github-actions
Copy link

github-actions bot commented Jan 17, 2023

Ember Test Audit comparison

epic/job-templates d4d6825 change
passes 1454 1458 +4
failures 0 0 0
flaky 0 0 0
duration 10m 43s 624ms 12m 04s 691ms +1m 21s 067ms

* refact: update assertion

* refact: test set-up
@ChaiWithJai ChaiWithJai merged commit 9652856 into epic/job-templates Jan 17, 2023
@ChaiWithJai ChaiWithJai deleted the job-templates/patch branch January 17, 2023 20:59
philrenaud added a commit that referenced this pull request Feb 2, 2023
* Extend variables under the nomad path prefix to allow for job-templates (#15570)

* Extend variables under the nomad path prefix to allow for job-templates

* Add job-templates to error message hinting

* RadioCard component for Job Templates (#15582)

* chore: add

* test: component API

* ui: component template

* refact: remove  bc naming collission

* styles: remove SASS var causing conflicts

* Disallow specific variable at nomad/job-templates (#15681)

* Disallows variables at exactly nomad/job-templates

* idiomatic refactor

* Expanding nomad job init to accept a template flag (#15571)

* Adding a string flag for templates on job init

* data-down actions-up version of a custom template editor within variable

* Dont force grid on job template editor

* list-templates flag started

* Correctly slice from end of path name

* Pre-review cleanup

* Variable form acceptance test for job template editing

* Some review cleanup

* List Job templates test

* Example from template test

* Using must.assertions instead of require etc

* ui: add choose template button (#15596)

* ui: add new routes

* chore: update file directory

* ui: add choose template button

* test: button and page navigation

* refact: update var name

* ui: use `Button` component from `HDS` (#15607)

* ui: integrate  buttons

* refact: remove  helper

* ui: remove icons on non-tertiary buttons

* refact: update normalize method for key/value pairs (#15612)

* `revert`: `onCancel` for `JobDefinition`

The `onCancel` method isn't included in the component API for `JobEditor` and the primary cancel behavior exists outside of the component. With the exception of the `JobDefinition` page where we include this button in the top right of the component instead of next to the `Plan` button.

* style: increase button size

* style: keep lime green

* ui: select template (#15613)

* ui: deprecate unused component

* ui: deprecate tests

* ui: jobs.run.templates.index

* ui: update logic to handle templates

* refact: revert key/value changes

* style: padding for cards + buttons

* temp: fixtures for mirage testing

* Revert "refact: revert key/value changes"

This reverts commit 124e95d.

* ui: guard template for unsaved job

* ui: handle reading template variable

* Revert "refact: update normalize method for key/value pairs (#15612)"

This reverts commit 6f5ffc9.

* revert: remove test fixtures

* revert: prettier problems

* refact: test doesnt need filter expression

* styling: button sizes and responsive cards

* refact: remove route guarding

* ui: update variable adapter

* refact: remove model editing behavior

* refact: model should query variables to populate editor

* ui: clear qp on exit

* refact: cleanup deprecated API

* refact: query all namespaces

* refact: deprecate action

* ui: rely on  collection

* refact: patch deprecate transition API

* refact: patch test to expect namespace qp

* styling: padding, conditionals

* ui: flashMessage on 404

* test: update for o(n+1) query

* ui: create new job template (#15744)

* refact: remove unused code

* refact: add type safety

* test: select template flow

* test: add data-test attrs

* chore: remove dead code

* test: create new job flow

* ui: add create button

* ui: create job template

* refact: no need for wildcard

* refact:  record instead of delete

* styling: spacing

* ui: add error handling and form validation to job create template (#15767)

* ui: handle server side errors

* ui: show error to prevent duplicate

* refact: conditional namespace

* ui: save as template flow (#15787)

* bug:  patches failing tests associated with `pretender` (#15812)

* refact: update assertion

* refact: test set-up

* ui: job templates manager view (#15815)

* ui: manager list view

* test: edit flow

* refact: deprecate column-helper

* ui: template edit and delete flow (#15823)

* ui: manager list view

* refact: update title

* refact: update permissions

* ui: template edit page

* bug: typo

* refact: update toast messages

* bug:  clear selections on exit (#15827)

* bug:  clear controllers on exit

* test: mirage config changes (#15828)

* refact: deprecate column-helper

* style: update z-index for HDS

* Revert "style: update z-index for HDS"

This reverts commit d3d87ce.

* refact: update delete button

* refact: edit redirect

* refact: patch reactivity issues

* styling: fixed width

* refact: override defaults

* styling: edit text causing overflow

* styling:  add inline text

Co-authored-by: Phil Renaud <phil.renaud@hashicorp.com>

* bug: edit `text` to `template`

Co-authored-by: Phil Renaud <phil.renaud@hashicorp.com>

Co-authored-by: Phil Renaud <phil.renaud@hashicorp.com>

* test:  delete flow job templates (#15896)

* refact: edit names

* bug:  set correct ref to store

* chore: trim whitespace:

* test: delete flow

* bug: reactively update view (#15904)

* Initialized default jobs (#15856)

* Initialized default jobs

* More jobs scaffolded

* Better commenting on a couple example job specs

* Adapter doing the work

* fall back to epic config

* Label format helper and custom serialization logic

* Test updates to account for a never-empty state

* Test suite uses settled and maintain RecordArray in adapter return

* Updates to hello-world and variables example jobspecs

* Parameterized job gets optional payload output

* Formatting changes for param and service discovery job templates

* Multi-group service discovery job

* Basic test for default templates (#15965)

* Basic test for default templates

* Percy snapshot for manage page

* Some late-breaking design changes

* Some copy edits to the header paragraphs for job templates (#15967)

* Added some init options for job templates (#15994)

* Async method for populating default job templates from the variable adapter

---------

Co-authored-by: Jai <41024828+ChaiWithJai@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants