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

Parse list value for terminado_settings #949

Merged
merged 5 commits into from
Sep 13, 2022

Conversation

krassowski
Copy link
Collaborator

@krassowski krassowski commented Aug 18, 2022

Fixes jupyterlab/jupyterlab#12540. Allows configuring terminado_settings from command line with:

--ServerApp.terminado_settings="shell_command=['/bin/zsh']"

or if users are willing to accept the default tokenization by shlex.split with:

--ServerApp.terminado_settings="shell_command='/bin/zsh'"

Are there tests for configuration settings?

For context it is used in: jupyter_server_terminals

@blink1073
Copy link
Contributor

This seems like a reasonable change for now. Ideally we'd have a custom parser for each of the allowed fields. Here's an example of a config setting test.

@Zsailer Zsailer added the bug label Aug 29, 2022
@krassowski krassowski force-pushed the fix-terminal-settings branch from 4919ac0 to 0fef4f1 Compare September 8, 2022 19:13
tests/test_terminal.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2022

Codecov Report

Merging #949 (33fcc01) into main (4a34e6a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #949   +/-   ##
=======================================
  Coverage   72.30%   72.30%           
=======================================
  Files          64       64           
  Lines        8136     8136           
  Branches     1358     1358           
=======================================
  Hits         5883     5883           
  Misses       1843     1843           
  Partials      410      410           
Impacted Files Coverage Δ
jupyter_server/serverapp.py 65.76% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@blink1073
Copy link
Contributor

blink1073
blink1073 previously approved these changes Sep 13, 2022
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks! Failures are unrelated

@krassowski
Copy link
Collaborator Author

Thanks! Test Minimum Versions failure is relevant (other tests also fail on #971). Should I:

  • mark the test case for skipping on older traitlets, or
  • bump the minimum version of traitlets to 5.4 (currently 5.1)?

@blink1073
Copy link
Contributor

Oh, the minimum version check is related. We'll need a version guard on the new test for the traitlets version.

@blink1073
Copy link
Contributor

mark the test case for skipping on older traitlets, or

This one 😄

@blink1073
Copy link
Contributor

Thanks again!

@blink1073 blink1073 merged commit dfbc5b5 into jupyter-server:main Sep 13, 2022
@vidartf
Copy link
Member

vidartf commented Nov 3, 2022

@krassowski Note that the test that was added was failing locally on Windows for me. This change to the test make it pass, but I'm not sure if this means the bug is fixed on Windows or not? #983 (comment)

@westurner
Copy link

Is there already a way to use busybox ash (or bash) compiled to WASM with this setting?

emscripten-forge could build busybox, bash, and zsh, I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to set --ServerApp.terminado_settings='shell_command=/bin/zsh'
6 participants