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

[1.x] Added configuration of gx-it-proxy to support path-based proxying #100

Merged
merged 6 commits into from
Mar 13, 2023

Conversation

natefoo
Copy link
Member

@natefoo natefoo commented Feb 13, 2023

Supersedes the 0.x-based #96.

Only adds --proxyPathPrefix if the Galaxy config option is set, which by my reading of galaxyproject/gx-it-proxy#14 should be valid, but correct me if I'm wrong @sveinugu.

Copy link
Contributor

@sveinugu sveinugu left a comment

Choose a reason for hiding this comment

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

Hi @natefoo. Sorry for the very late review on this.

We (@morj-uio and myself) have now reviewed your merge of our PR and have added some comments in the code. We had to spend some time to remember the details, but it is now relatively clear to us that there is no reason to not always set --proxyPathPrefix, as the path-based IT solution should really be the fallback when domain-based ITs are not available (i.e. when there is no wildcard DNS) and not the other way around. For now, the use of path-based IT mapping can be regulated on a tool-by-tool basis (by setting requires_domain to False for path-based) until we are confident that this works as intended. In any case, the interactivetools_base_path is not the correct config to check, as the default is /.

gravity/state.py Outdated Show resolved Hide resolved
@natefoo
Copy link
Member Author

natefoo commented Mar 10, 2023

@sveinugu ok, I think this does it as discussed, can you confirm this looks right to you?

Copy link
Contributor

@sveinugu sveinugu left a comment

Choose a reason for hiding this comment

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

I think one of the tests is checking too little. Other than that: LGTM

tests/test_process_manager.py Outdated Show resolved Hide resolved
@sveinugu
Copy link
Contributor

I believe tests are failing due to lack of version bump in gx-it-proxy: galaxyproject/gx-it-proxy#16. Bumped versions should be harmonized across galaxy, gravity, and gx-it-proxy.

@natefoo
Copy link
Member Author

natefoo commented Mar 10, 2023

Ok, the new version of gx-it-proxy is published, but the tests are now failing due (at least in part) to galaxyproject/galaxy#15765, so we can rerun once that is fixed.

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.

2 participants