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

fix: using default_scheduler_url as a mounting point for not root pat… #3216

Merged
merged 5 commits into from Feb 23, 2023

Conversation

franckOL
Copy link
Contributor

@franckOL franckOL commented Jan 3, 2023

…h. fix #3213

Signed-off-by: ROUDET Franck INNOV/IT-S franck.roudet@orange.com

Description

1 files modified to change the build of URL.
Keep full base path from default_scheduler_url.
2nd attempt for that: more cleaner approach.

Motivation and Context

Fixes:

Have you tested this? If so, how?

I'm able to test:

  • With or Without scheduler-url set
  • With or Without scheduler-url not at root

fix spotify#3213

Signed-off-by: ROUDET Franck INNOV/IT-S <franck.roudet@orange.com>
@franckOL franckOL requested review from dlstadther and a team as code owners January 3, 2023 15:09
@franckOL
Copy link
Contributor Author

franckOL commented Jan 3, 2023

@dlstadther , I found a better solution and made a cleaner pull request.

@franckOL
Copy link
Contributor Author

@dlstadther , Is it OK ? I don't know to help more.
I use the feature every day.

luigi/rpc.py Outdated
@@ -59,7 +59,7 @@ def _urljoin(base, url):
parsed = urlparse(base)
scheme = parsed.scheme
return urlparse(
urljoin(parsed._replace(scheme='http').geturl(), url)
urljoin(parsed._replace(scheme='http').geturl(), url if parsed.path == '' else parsed.path + url)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure i understand the end result impact here.

I also don't understand why you do parsed.path + url instead of url + parsed.path.

It would be preferred if you could provide a unittest in https://github.com/spotify/luigi/blob/master/test/rpc_test.py to show this behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am trying to look at local testing by looking at CONTRIBUTING.rst stuff.
And failing to use tox.
For exemple:

tox -e py37-core test/rpc_test.py
# gives
usage: tox [-h] [--colored {yes,no}] [-v | -q] [--exit-and-dump-after seconds] [-c file] [--workdir dir] [--root dir] [--runner {virtualenv}] [--version] [--no-provision [REQ_JSON]] [--no-recreate-provision] [-r] [-x OVERRIDE]
           {run,r,run-parallel,p,depends,de,list,l,devenv,d,config,c,quickstart,q,exec,e,legacy,le} ...
tox: error: unrecognized arguments: test/rpc_test.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have also:

tox -e py39-core
py39-core: failed with pass_env values cannot contain whitespace, use comma to have multiple values in a single line, invalid values found 'USER JAVA_HOME POSTGRES_USER DATAPROC_TEST_PROJECT_ID GCS_TEST_PROJECT_ID GCS_TEST_BUCKET GOOGLE_APPLICATION_CREDENTIALS TRAVIS_BUILD_ID TRAVIS TRAVIS_BRANCH TRAVIS_JOB_NUMBER TRAVIS_PULL_REQUEST TRAVIS_JOB_ID TRAVIS_REPO_SLUG TRAVIS_COMMIT CI DROPBOX_APP_TOKEN DOCKERHUB_TOKEN GITHUB_ACTIONS OVERRIDE_SKIP_CI_TESTS'
  py39-core: FAIL code 1 (0.00 seconds)
  evaluation failed :( (0.03 seconds)

Copy link
Collaborator

Choose a reason for hiding this comment

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

What version of tox do you have installed?
I know that our build process is set to be tox<4.0.

I just created a fresh venv and ran the following:

$ python --version
Python 3.9.12

$ python -m venv ~/venv/luigi-39
$ ~/venv/luigi-39/bin/python -m pip install "tox<4.0"
...
Successfully installed distlib-0.3.6 filelock-3.9.0 packaging-23.0 platformdirs-2.6.2 pluggy-1.0.0 py-1.11.0 six-1.16.0 tomli-2.0.1 tox-3.28.0 virtualenv-20.17.1

$ ~/venv/luigi-39/bin/tox -e py39-core test/rpc_test.py
=============================================== test session starts ===============================================
platform darwin -- Python 3.9.12, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 -- /Users/dstadther/code/luigi/.tox/py39-core/bin/python
cachedir: .tox/py39-core/.pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase('/Users/dstadther/code/luigi/.hypothesis/examples')
rootdir: /Users/dstadther/code/luigi, configfile: tox.ini
plugins: cov-2.12.1, hypothesis-6.65.2
collected 207 items
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx.

Tests are OK now.

I try to explain better what I propose what is new is the 4 last line:

RemoteScheduler base target URL full path
http://zorg.com api/123 http://zorg.com/api/123
http://zorg.com /api/123 http://zorg.com/api/123
http://zorg.com/ api/123 http://zorg.com/api/123
http://zorg.com/ /api/123 http://zorg.com/api/123
http://zorg.com/subpath api/123 http://zorg.com/api/subpath/123
http://zorg.com/subpath /api/123 http://zorg.com/api/subpath/123
http://zorg.com/subpath/ api/123 http://zorg.com/api/subpath/123
http://zorg.com/subpath/ /api/123 http://zorg.com/api/subpath/123

When you have that, you can can an http reverse proxy routing to luigi whith setting default_scheduler_url to your subpath (ex: http://zorg.com/subpath ).

GUI is yet ready for that, LuigiAPI is initialized with relative URL - new LuigiAPI("../../api") -

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think new tests are OK, have codeconv/changes not ok, bot not related to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlstadther . Is it OK for explanation too ?

ROUDET Franck INNOV/IT-S and others added 4 commits January 18, 2023 16:52
Signed-off-by: ROUDET Franck INNOV/IT-S <EFRE6460@orange.com>
Signed-off-by: Franck Roudet <franck.roudet@orange.com>
@dlstadther dlstadther merged commit c38cb08 into spotify:master Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using default_scheduler_url as a mounting point (not root http://address/mount) behind proxy not working
2 participants