-
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
api-on-the-fly - part 4 - mutations #385
Conversation
* enums * imput objects * also add some other cylc types
* FormInput should be a configurable interface supporting different compoents * For now just move the config to another file and stick it into the component's data
I think this must be the last or penultimate PR for AOF in thr UI?? 🎉 |
Three bullet points left, but getting close. Auto-completing task names and cycle points will be fun. |
Finally managed to compile peek after manually hacking some of the generated files to pick up the correct environment, something is jiggered in the recording/conversion, meh, screenshots will have to do for now. |
I know the mutations view is temporary ... but using waiting and succeeded task status (name and icons) for the command status could be a bit confusing. Maybe just changing the wording, something like "command pending" and "command succeeded"? |
@oliver-sanders not working on infinite tree right now, so happy to review once the conflicts are solved 🎉 |
abf864b
to
c417111
Compare
7f9e4ec
to
741f6f1
Compare
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.
LGTM, tests as working.
@kinow I've deconflicted this branch - if you can squeeze in a quick review we can merge it. @oliver-sanders is away today.
Note the conflict resolution was not trivial, and I didn't entirely know what I was doing. All in one commit in case @oliver-sanders needs to check it later 741f6f1 |
Sure thing @hjoliver, reviewing it right now. |
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 comments on typos and default parameters, but no blockers I think 👍
This comment has been minimized.
This comment has been minimized.
Leaving final sign off to @oliver-sanders so that he can confirm the conflict resolution didn't miss anything, and that there's nothing that needs to be done after my comments ☝️ or should we just merge it now @hjoliver as there will be a part 5 PR anyway? |
That definitely worked for me - presumably you used the drop-down list at the top of the page (to choose "hold")? |
🤦♂ working as expected! Just hid my old comment. |
Maybe my rushed conflict resolution attempt - in the hope of getting this in today - was a bit premature! @oliver-sanders feel free to back out my commit if it's easier for you to do the resolution from scratch. (I think I probably got it right, plus it does pass the unit tests and it tests as working for me, but not sure if you intended to keep some of the earlier "temporary" stuff in, and I confess that some of the "example page" stuff apparent in @kinow 's screenshot above isn't clear to me). |
Actually, after a bit of head-scratching from myself and @kinow above ... I know the "mutations view" is destined to be a temporary thing, but:
|
Just caught up with the comments...
The "Sample Mutation" is auto-generated from the schema
|
So, we should keep this in somehow (and documented) for use by developers in future. (Development mode only?) |
Yep, somewhere, not quite sure where. |
Conflicts: src/components/cylc/workflow/index.js src/components/graphqlFormGenerator/FormGenerator.vue src/components/graphqlFormGenerator/FormInput.vue src/components/graphqlFormGenerator/components/List.vue src/components/graphqlFormGenerator/mixins.js src/views/Mutations.vue tests/unit/components/graphqlFormGenerator/formgenerator.vue.spec.js tests/unit/utils/graphql.spec.js
741f6f1
to
d6080a2
Compare
Wasn't the easiest merge ever, especially for a non-author! Found a couple of small errors so re-merged diffing against your attempt for validation. |
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.
Had a look at the last three commits added. Looking good, re-approving +1
23ee276
to
5242390
Compare
@oliver-sanders at least my attempt passed the linter 😁
|
5242390
to
15408f8
Compare
The subtle differences between Python and JavaScript make it really horrible to quickly switch between the two. |
Addresses the "executing mutations" point in #339
Note - *Built atop #382 *
Note - Works with the data types defined in cylc/cylc-flow#3469
Add the ability to run mutations from the temporary Mutations view.
Wraps the mutations view up as a Lumino widget in the workflows view, still a a temporary measure but a good POC.
We could automatically fill in the workflow id field based on the current workflow but I think that'll have to wait for the next phase.
TODO:
What you should expect to see:
<user>|<reg>
).Example:
From the UI:
In the suite log:
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.