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

Await rendered template if neccesary #40

Merged
merged 2 commits into from
Feb 9, 2021

Conversation

TimoRoth
Copy link
Contributor

Latest release of jupyterhub made render_template return a coroutine: jupyterhub/jupyterhub@be211a4

So it needs to be awaited now, or else self.write throws an error.
To keep backwards compatibility, I first check of the result is indeed awaitable.
Could also just drop this check and always await it, if assuming the use of the latest version of jupyterhub is fine.

@jtpio
Copy link
Member

jtpio commented Jan 8, 2021

Thanks a lot @TimoRoth for this, looks good!

I wonder if it would be possible to test with multiple versions of JupyterHub on CI. Maybe that would require adding a new entry in the matrix for the jupyterhub version, and possibly a new step in the workflow?

https://github.com/plasmabio/tljh-repo2docker/blob/master/.github/workflows/test.yml

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio
Copy link
Member

jtpio commented Feb 9, 2021

Could also just drop this check and always await it, if assuming the use of the latest version of jupyterhub is fine.

We probably want to keep this check for now, since TLJH currently uses 1.2.2:

https://github.com/jupyterhub/the-littlest-jupyterhub/blob/18efee665aebd6c59334685eb9929f823ad649b0/tljh/installer.py#L225

@jtpio jtpio merged commit 3545a64 into plasmabio:master Feb 9, 2021
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.

3 participants