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

Add GraphQL support #11

Closed
kinow opened this issue Mar 20, 2019 · 10 comments
Closed

Add GraphQL support #11

kinow opened this issue Mar 20, 2019 · 10 comments
Assignees
Milestone

Comments

@kinow
Copy link
Member

kinow commented Mar 20, 2019

As per discussion in Riot, we are now ready to start porting the work from @dwsutherland with GraphQL + Flask to here, with GraphQL + Tornado.

As both the Flask and the Tornado implementations are provided as extensions of the Python Graphene project, it should be fairly easy to port most of the code.

Flask has an approach of a slim core, with extensions installed via pip for things like CORS, HTTP Basic, etc. Whereas Tornado includes a slim core too, but without official modules for things like CORS or some authentication mechanisms.

So this may be a bit of a challenge.

See:

@kinow kinow self-assigned this Mar 20, 2019
@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

@dwsutherland looks to me like there are small changes required for GraphQL + Flask. The majority of the changes are in Cylc to have the right objects required to populate the GraphQL response.

Q: about the schema.py.

I believe this is where we have the complete API contract, available through our GraphQL endpoint for clients.

Where should we put it? I think it will go to cylc/cylc. The reason for that is to avoid cyclic dependency between the Python modules.

This current project, cylc/cylc-jupyterhub, may use cylc/cylc as dependency, to start commands, scan suites, etc. So we have a dependency cylc-jupyterhub --- to ---> cylc.

But if we have schema.py here, I think we would have to import it from cylc/cylc. Causing a cyclic/circular dependency. Which looks like we can try to hack our way out of circular dependencies in Python by changing where/how we import stuff, but it would still be harder to maintain.

Another alternative would be to have cylc/cylc/, cylc/cylc-jupyterhub, and a cylc/cylc-graphql or something module, where both cylc and cylc-jupyterhub would depend on.

WDYT?

kinow pushed a commit to kinow/cylc-uiserver that referenced this issue Mar 20, 2019
@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

Work started here: https://github.com/kinow/cylc-jupyterhub/tree/add-graphql

So far have only added graphene-tornado. Created a new virtualenv, then installed through pip install -e . with no issues. Already have the base dependency for the GraphQL endpoint. Just need to confirm where the schema will be (see comment above)

@dwsutherland
Copy link
Member

dwsutherland commented Mar 20, 2019

@dwsutherland looks to me like there are small changes required for GraphQL + Flask. The majority of the changes are in Cylc to have the right objects required to populate the GraphQL response.

This is a breaking change, and we will no longer be able to use the Flask branch, we will divorce from it here on out. Yeah, we need to setup the feed from cylc8.

Q: about the schema.py.

I believe this is where we have the complete API contract, available through our GraphQL endpoint for clients.

Where should we put it? I think it will go to cylc/cylc. The reason for that is to avoid cyclic dependency between the Python modules.

This current project, cylc/cylc-jupyterhub, may use cylc/cylc as dependency, to start commands, scan suites, etc. So we have a dependency cylc-jupyterhub --- to ---> cylc.

But if we have schema.py here, I think we would have to import it from cylc/cylc. Causing a cyclic/circular dependency. Which looks like we can try to hack our way out of circular dependencies in Python by changing where/how we import stuff, but it would still be harder to maintain.

Another alternative would be to have cylc/cylc/, cylc/cylc-jupyterhub, and a cylc/cylc-graphql or something module, where both cylc and cylc-jupyterhub would depend on.

WDYT?

The schema is no longer needed in the suite (we will transport the full field objects) and resolved them in the UI Server. So I'll make a pull request to cylc/cylc-jupyterhub when I integrate it after your done with Tornado (unless there's good argument for having GraphQL at both ends)..

Just use a hello world schema until then..

All good?

@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

Just use a hello world schema until then..

Oh, much easier for now. Thanks @dwsutherland !

kinow added a commit to kinow/cylc-uiserver that referenced this issue Mar 20, 2019
@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

Done.

image

@kinow kinow mentioned this issue Mar 29, 2019
@kinow
Copy link
Member Author

kinow commented Mar 29, 2019

Tested WebSockets today. Used the simple example from the Tornado documentation, which just prints back whatever you say on the message.

image

Not possible to use that endpoint in a browser, unless you write JS code. For now the WebSocket is insecure, but we should try to use a secure WebSocket in production.

Screenshot_2019-03-29_13-41-02

Then tested without a cookie/authentication.

image

And that worked, as the endpoint was not extending JupyterHub's HubOAuthenticated class/trait.

Extending the right class, we can't connect without logging in:

image

And finally, once logged in, the WebSocket worked again! 🎉

image

So I believe we are all good with WebSockets support for Tornado, with integration with the Hub. @dwsutherland I think the only thing missing now is the graphql-python/graphql-ws#25 👍

@dwsutherland
Copy link
Member

@kinow - Yes, it can't use the same HTTP authentication.. but authentication is done at the hub right? else we may be able to use other types, or bespoke (as you mentioned)

@kinow
Copy link
Member Author

kinow commented Mar 29, 2019

Authentication done in the hub. We can still change who is authorised to see what however.

@kinow
Copy link
Member Author

kinow commented Apr 11, 2019

Merged. Remaining work in #14, and later more work on data model.

@kinow kinow closed this as completed Apr 11, 2019
@kinow kinow added this to the 0.1 milestone Sep 10, 2019
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

No branches or pull requests

2 participants