-
Notifications
You must be signed in to change notification settings - Fork 36
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
uplift ci without JLab upgrade #165
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #165 +/- ##
=======================================
Coverage ? 40.50%
=======================================
Files ? 23
Lines ? 1153
Branches ? 143
=======================================
Hits ? 467
Misses ? 643
Partials ? 43 ☔ View full report in Codecov by Sentry. |
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.
Two comment points, but I will merge and iterate on them afterwards in order to have things progressing.
@@ -42,8 +42,9 @@ def _load_jupyter_server_extension(serverapp): | |||
warnings.warn(_mm_config_warning_msg) | |||
return | |||
|
|||
serverapp.contents_manager_class = MetaManager |
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.
serverapp.contents_manager_class = MetaManager |
By setting this here, it means we cannot use the setting system to configure the CM class as e.g. a subclass of MetaManager. Normally I would expect the config file (jupyter_server_config.json) to be enough. Alternatively, we could do a
if not isinstance(serverapp.contents_manager_class, MetaManager): ...
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.
I couldn't get it working with just the config file, the type was set but it wasn't actually using it unless this override was present
"fs-s3fs>=1.1.1", | ||
"fs.smbfs>=0.6.3", | ||
"jupyterlab>=3.5,<4", | ||
"jupyter_server>=2,<3", |
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.
I would want to keep these at the old version for the next release cycle, and then bump them directly after.
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.
"fs>=2.4.11",
"fs-s3fs>=1.1.1",
"fs.smbfs>=0.6.3",
"jupyterlab>=3.0.0",
"jupyter_server>=1.8.0",
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.
I don't think the server upgrade is strictly necessary, but the jupyterlab scoping is now that 4 is out
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.
Ah, to clarify for the lab one, I meant the lower bound. Keeping the upper bound for the upcoming release makes sense 👍
@timkpaine Since the pipelines are running with your account, e.g. here: https://dev.azure.com/tpaine154/jupyter/_build/results?buildId=3017&view=logs&jobId=cea277d1-520e-58b3-21c9-6f6931bd2abd , do you think you could give this SO answer a try? Sorry if this is missing the mark, I don't know much about configuring Azure pipelines. |
Thanks Tim! 👍 |
separate from #162
temporary fix for #167
fixes #72