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

Server detection configuration files are not passed to us from config directories #535

Closed
krassowski opened this issue Feb 20, 2021 · 4 comments · Fixed by #538
Closed
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@krassowski
Copy link
Member

krassowski commented Feb 20, 2021

Placing LSP server config in jupyter_notebook/server_config.d does not seem to work. See #534 for details; using jupyter_server_config.json still works. The server apps detects the json files:

[ServerApp] Paths used for configuration of jupyter_server_config:
etc/jupyter/jupyter_server_config.d/pyright.json

in several passes. If this is an upstream issue, it needs to be looked into; if this is something at our end, we need to fix/update the docs.

@krassowski krassowski added bug Something isn't working documentation Improvements or additions to documentation labels Feb 20, 2021
@krassowski krassowski changed the title Language detection configuration files are not passed to us from jupyter_notebook/server_config.d directories Sever detection configuration files are not passed to us from jupyter_notebook/server_config.d directories Feb 20, 2021
@krassowski krassowski changed the title Sever detection configuration files are not passed to us from jupyter_notebook/server_config.d directories Sever detection configuration files are not passed to us from config directories Feb 20, 2021
@krassowski
Copy link
Member Author

Any immediate thoughts @bollwyvl?

@bollwyvl
Copy link
Collaborator

Yeah, it's weird it doesn't find it... but we should definitely not be distributing a jupyter_(server|notebook)_config.json, as that gets updated by command line stuff (serverextension enable, etc)... nothing that has a cardinality of 1 can be shared by more than one extension, and as we've seen, nobody runs just our stuff.

I'll have a look at validating the setup, but getting it work work with conf.d is critical.

Basic plan:

  • make a any_conf_d fixture which gives all the permutations across server and notebook
  • validate that the manager loads them

At present, we don't actually instantiate the parent application directly (which is part of why the migration was so straightforward).

We might also need to make the smoke tests more angry.

@bollwyvl bollwyvl changed the title Sever detection configuration files are not passed to us from config directories Server detection configuration files are not passed to us from config directories Feb 20, 2021
@bollwyvl
Copy link
Collaborator

Oh, yeah, it is an upstream issue... I've had to work around this before on jupyterlab-starters.

I don't think the contract was ever, "you can configure any HasTraits", just "you can configure NotebookApp or ServerApp". It may not even be feasible for the general case.

When conf.d was added to notebook (mainly to remove the need for install-time executed code), there was pushback against actually adding it to the underlying triatlets.config system, as it doesn't actually make a claim about where config files go on disk, so all the Application subclasses have to implement it themselves.

I'll adapt my approach from starters, and while i'm at it, I could add an API route to reload the config, such that folk can put a file in place and have new servers show up... probably won't touch any JS on the PR, though...

@bollwyvl
Copy link
Collaborator

#538 is looking good... didn't mess about with dynamic re-configuration, there's probably a lot to consider about that, and actually having the behavior we've had in the docs forever seems more important than that particular async design discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants