-
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 2 - mutation association #359
api-on-the-fly - part 2 - mutation association #359
Conversation
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.
Form generation working. Could you explain what are the associations? I chose holdWorkflow
, but could choose other types, and not only workflow in association. So not quite sure I understand how that part will work.
Some consoler errors
Debugging, it shows that the this.value
value is a String I think?
Everything else works, nice form generation! 🎉 As the whole code is new, +1 LGTM and we can discuss any changes afterwards if you prefer 👍
(Or are associations going to be restricted later, and only for now I can associate anything with any mutation?) |
The "associations" work is simply about introspecting the arguments of mutations to find out what Cylc things the mutation operates on (i.e. workflows, cycle points, tasks, jobs). The result is a mapping (e.g. the broadcast mutation works on multiple workflows and [optionally] multiple cycle points). This will be used in a future PR to achieve this:
|
So this is the same traceback is the same issue mentioned in #357, I had thought this was caused by error in the GraphQL schema (from Cylc Flow) but it turns out it isn't. GraphQL introspections provide "default values" in JSON format so lists come through as strings (e.g. This PR just adds associations and doesn't touch the form generation logic (which is where the error is coming from). |
b5a488b
to
55bdaa8
Compare
@wxtim I've put you down for review 2, this is a much quicker review than the last one. |
Change log and tests? |
55bdaa8
to
139e08e
Compare
Added unit tests, change log not helpful as the mutations view (as is in this PR), will not make release. |
139e08e
to
4fd060b
Compare
It will make the next (alpha-2) release. By 8.0.0 we should have another change long entry that removes it. (At least, that's how we are managing pre-8.0.0 changes, in my mind!) |
You seem to have a bunch of minor quoting issues (via the linter) in the new tests. |
I'll put a changelog in on the most contemporary aotf PR for a2. |
4fd060b
to
a2c3501
Compare
Doesn't fail in my env, different linter versions perhaps, should be fixed. |
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
Addresses the fourth point of #339
Note - Built on top of #357now mergedNote - Works with the data types defined in cylc/cylc-flow#3469
Use the argument types defined in the GraphQL schema to associate mutations with "Cylc Objects" by which I mean workflows, cycle point, tasks, etc.
This mapping will be used when you right-click on something (say a task in the tree view) to work out which mutations are relevant to the thing you clicked on.
At the moment this is built into the temporary Mutations view.
Example
TODO
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.