-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Conversation
fix spotify#3213 Signed-off-by: ROUDET Franck INNOV/IT-S <[email protected]>
@dlstadther , I found a better solution and made a cleaner pull request. |
@dlstadther , Is it OK ? I don't know to help more. |
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
...
There was a problem hiding this comment.
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") -
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
Signed-off-by: ROUDET Franck INNOV/IT-S <[email protected]>
Signed-off-by: Franck Roudet <[email protected]>
…h. fix #3213
Signed-off-by: ROUDET Franck INNOV/IT-S [email protected]
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:
http://address/mount
) behind proxy not working #3213Have you tested this? If so, how?
I'm able to test: