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

set default config dir name #504

Merged
merged 1 commit into from
May 13, 2021
Merged

Conversation

minrk
Copy link
Contributor

@minrk minrk commented May 3, 2021

default is not a kwarg of the traitlet constructor (default_value is)

The result was a warning on every jupyter server startup that tag metadata was being stored, but this is deprecated. The default value was not being set.

`default` is not a kwarg of the traitlet constructor (default_value is)

The result was a warning on every jupyter server startup that tag metadata was being stored,
but this is deprecated. The default value was *not* being set.
@codecov-commenter
Copy link

codecov-commenter commented May 3, 2021

Codecov Report

Merging #504 (1c00e8c) into master (5e77b73) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
- Coverage   77.68%   77.64%   -0.05%     
==========================================
  Files         106      106              
  Lines        9276     9276              
  Branches     1001     1001              
==========================================
- Hits         7206     7202       -4     
- Misses       1711     1713       +2     
- Partials      359      361       +2     
Impacted Files Coverage Δ
jupyter_server/services/config/manager.py 100.00% <ø> (ø)
jupyter_server/services/contents/filemanager.py 65.48% <0.00%> (-0.84%) ⬇️
jupyter_server/serverapp.py 67.02% <0.00%> (-0.22%) ⬇️
jupyter_server/services/kernels/kernelmanager.py 81.09% <0.00%> (+0.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e77b73...1c00e8c. Read the comment docs.

@minrk
Copy link
Contributor Author

minrk commented May 3, 2021

It's a bit unclear if this is the right thing to do, vs remove the setting that doesn't do anything - this changes the default config path to $config_path/serverconfig instead of the current (unintended, I think) $config_path itself. This might break things, but I'm not 100% sure exactly when the default is used. Server extensions, for instance, I believe explicitly set the secondary properties (read/write_config_path), meaning they shouldn't be affected.

However, the REST API for server-side config manager should be affected by this, I think.

@blink1073 blink1073 added this to the 1.7 milestone May 3, 2021
@Zsailer
Copy link
Member

Zsailer commented May 3, 2021

For reference, this is hardcoded in jupyter/notebook: https://github.com/jupyter/notebook/blob/8ca28879202296176d8d9b6ba76704462d1dcd1b/notebook/services/config/manager.py#L48

@blink1073 blink1073 modified the milestones: 1.7, 1.8 May 10, 2021
@Zsailer Zsailer merged commit 284c938 into jupyter-server:master May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants