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

Add base_url capability to tljh-config #623

Merged
merged 9 commits into from Nov 5, 2020
Merged

Add base_url capability to tljh-config #623

merged 9 commits into from Nov 5, 2020

Conversation

jeanmarcalkazzi
Copy link
Contributor

@jeanmarcalkazzi jeanmarcalkazzi commented Oct 19, 2020

This pull request:

A function update_base_url now sets c.JupyterHub.base_url to the requested value, and correctly checks for it during the check_hub_ready function.

  • Add / update documentation
  • Add tests

@welcome
Copy link

welcome bot commented Oct 19, 2020

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@jeanmarcalkazzi
Copy link
Contributor Author

@GeorgianaElena here's the fix for @ikhoury 's issue #616
Some tests are failing here due to broken master.

Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @jeanmarcalkazzi, it looks great 🎉
What do you think about adding a small integration test in test_hub.py to check if a user can login using the new hub url and can start its server?

Update: Don't worry about the failing upgrade test, I don't think it's related to this PR. There is a memory limit issue that keeps coming back.

Update2: @jeanmarcalkazzi, I edited your initial comment, so that when this PR is merged, all the related issues to be closed. Hope that's ok.

@jeanmarcalkazzi
Copy link
Contributor Author

I will do so in the next few days!
Any specific requirement/aspect to keep in mind for testing?
@GeorgianaElena

@GeorgianaElena
Copy link
Member

The test should be similar with what happens along these lines: https://github.com/jupyterhub/the-littlest-jupyterhub/blob/master/integration-tests/test_hub.py#L27-L41, setting in addition the base_url.
One thing to keep in mind, all the tests in test_hub.py run in the same container, so I'd suggest unsetting the new base_url after the user server started, so that the other tests won't fail.

@jeanmarcalkazzi
Copy link
Contributor Author

CircleCI output:

WARNING: Your kernel does not support swap limit capabilities or the cgroup is not mounted. Memory limited without swap.
b84998b985cfd0e8f45141c9dd0a599ec54c6e313430295724a722be44e0d0de
^@^@Checking if TLJH is already installed...
Setting up hub environment
Installed python & virtual environment
Set up hub virtual environment
Setting up TLJH installer...
Upgraded pip
Setup tljh package
Starting TLJH installer...
Granting passwordless sudo to JupyterHub admins...
Setting up user environment...
Downloading & setting up user environment...
Traceback (most recent call last):
  File "/usr/lib/python3.6/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/opt/tljh/hub/lib/python3.6/site-packages/tljh/installer.py", line 530, in <module>
    main()
  File "/opt/tljh/hub/lib/python3.6/site-packages/tljh/installer.py", line 502, in main
    ensure_user_environment(args.user_requirements_txt_url)
  File "/opt/tljh/hub/lib/python3.6/site-packages/tljh/installer.py", line 278, in ensure_user_environment
    'conda==' + conda_version
  File "/opt/tljh/hub/lib/python3.6/site-packages/tljh/conda.py", line 108, in ensure_conda_packages
    ] + packages).decode()
  File "/usr/lib/python3.6/subprocess.py", line 356, in check_output
    **kwargs).stdout
  File "/usr/lib/python3.6/subprocess.py", line 438, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['/opt/tljh/user/bin/python', '-m', 'conda', 'install', '-c', 'conda-forge', '--json', '--prefix', '/opt/tljh/user', 'conda==4.8.1']' died with <Signals.SIGKILL: 9>.
Traceback (most recent call last):
  File ".circleci/integration-test.py", line 189, in <module>
    main()
  File ".circleci/integration-test.py", line 173, in main
    run_test(image_name, args.test_name, args.bootstrap_pip_spec, args.test_files, args.upgrade, args.installer_args)
  File ".circleci/integration-test.py", line 108, in run_test
    f'python3 /srv/src/bootstrap.py {installer_args}'
  File ".circleci/integration-test.py", line 71, in run_container_command
    ], check=True)
  File "/usr/lib/python3.6/subprocess.py", line 438, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['docker', 'exec', '-it', 'basic-tests', '/bin/bash', '-c', 'python3 /srv/src/bootstrap.py ']' returned non-zero exit status 1.

Exited with code exit status 1
CircleCI received exit code 1

@GeorgianaElena I think this is related to the memory issue you mentioned.

@GeorgianaElena
Copy link
Member

I believe you're right @jeanmarcalkazzi 😕 I will investigate this tomorrow. Thank you!

@jeanmarcalkazzi
Copy link
Contributor Author

Changes done.
@GeorgianaElena let me know if I can help with the debugging of this memory issue!

Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

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

Hey @jeanmarcalkazzi 👋 The tests were fixed, so I've rebased the PR and now all the bots are happy and everything is green 🥳

I'll go ahead and merge this now. Thanks a lot for your work and patience 🌻

@GeorgianaElena GeorgianaElena merged commit bff3114 into jupyterhub:master Nov 5, 2020
@welcome
Copy link

welcome bot commented Nov 5, 2020

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

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