-
Notifications
You must be signed in to change notification settings - Fork 27
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
Edit Runtime #1144
Edit Runtime #1144
Conversation
- Allow a mutation argument type to be associated with a cylc object type (cycle, task etc.) without auto filling that object into the form input - Add icon for Edit Runtime
Uses introspection to expand fields of the query if none specified
Update introspection query response in mock service
Avoids need to manually reset state of form
Because that's the order they appear in the broadcast
Simplify help icon logic - it now shows help on hover instead of click
Adapt AOTF's handling of 'INPUT_OBJECT' for 'OBJECT' seeing as the former is not used?
8a298a2
to
66f174a
Compare
66f174a
to
43c00d1
Compare
- Remain open with loading spinner in the submit button while awaiting the mutation response - Show error snackbar and stay open on failed submission - Show warning snackbar and stay open if the Edit-Runtime form does not have any changes on submission
43c00d1
to
12e02ad
Compare
I had a go at deconflicting with graph view (#1108) here: https://github.com/MetRonnie/cylc-ui/tree/edit-runtime-deconflict After #1108 is merged you may want to push this branch to the same commit as the deconflict branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional Review
- Ran some workflows, and played with them.
- Fixed broken scripts and triggered.
- Triggered scripts on SPICE with different directives.
- Had a look at the code FWIW
Questions
- Does the pencil do anything different to clicking on this menu item? If not, can we ghost it?
- Might I be able to control environment filters. This could be an add on. I'd also be happy not to - I'm not sure I'd want to encourage use of them.
- I rather expected to be able to edit then trigger - is there a reason this isn't possible?
- Has there been any thought to validation of any of the fields?
Like with other commands where the form is needed before submitting the mutation, there is no difference between clicking on the name and clicking on the pencil. I think this is fine.
Sorry, I'm not sure what environment filters are.
It was decided triggering would be performed separately. I think we discussed this in October's videoconference. See #1104 (comment) although the reasoning isn't recorded there.
Not yet, but it can be added later. |
Allow stuff through from the scheduler environment to the task env, or block it. #notapriority |
Ah right, it's a runtime section. It isn't included in the GraphQL schema, so that's a cylc-flow question/issue. |
One problem I have just noticed is that if you open the mutations menu for a task, then wait for it to finish before clicking "edit runtime", then the form remains as a skeleton loader and you get in the console:
Not a big problem though as you can just click cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a lot of reviewing left to do but looking good so far. Functionality is pretty slick 👍
<!-- the mutation title --> | ||
<h3 | ||
style="text-transform: capitalize;" | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor indentation niggles in this file:
<!-- the mutation title --> | |
<h3 | |
style="text-transform: capitalize;" | |
> | |
<!-- the mutation title --> | |
<h3 | |
style="text-transform: capitalize;" | |
> |
@@ -198,10 +202,13 @@ export default { | |||
} | |||
return this.mutations | |||
}, | |||
isFamily () { | |||
// TODO: better way of checking if a 'task' is actually a family? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really safe but ATM all views request isHeld
so it's fine for the mo.
As of the Graph View PR you can use node.type
in all situations, it will be one of:
task
family
cycle
(can be considered a family although you may need to add/root
on the end)
}, | ||
|
||
async submit () { | ||
const tokens = new Tokens(this.cylcObject.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post Graph View this is available as node.tokens
.
@@ -141,6 +99,70 @@ export default { | |||
|
|||
return ret | |||
} | |||
}, | |||
|
|||
render (createElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of interest what was the motivation for moving from <template>
to render()
? I can't see anything that looks like it couldn't have been done with <template>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template-based way became a bit awkward for passing in slot props to the append-outer
slot, due to an issue in Vuetify v2 (sorry can't find the GH link). Basically, doing it with a render function avoids the need to duplicate the slot with an if-else.
@oliver-sanders Happy for you to take over and push changes to this branch |
|
I have raised the deconflict branch Ronnie prepared in anticipation of the graph-view/central-data-store PR being merged as a new PR here - #1160 |
Superseded by #1160 |
These changes close #1104
[runtime]
settings for a task/family. Any edits made will be transmitted to the workflow viabroadcast
.Needs
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.