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

feat: support prefix/route when managed backend is behind a corporate… #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeroenflvr
Copy link

Support for a route/prefix when the presto backend is behind a corporate firewall on a route and presto is not managed by you.

@swapsmagic
Copy link

Can you add some intergration/unit test for the same?

Comment on lines +347 to +352
pattern = r"(http[s]?://[^/]+)(/.*)"
try:
return re.sub(pattern, r"\1" + self._prefix + r"\2", self._next_uri)
except TypeError:
logger.error("Unable to update url with prefix. "
"Continuing without prefix.")

Choose a reason for hiding this comment

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

Instead of failing during query execution, does it make sense to raise the error during the initialization if the _prefix provided doesn't match the regex?

Copy link

@swapsmagic swapsmagic left a comment

Choose a reason for hiding this comment

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

Adding some unit test/integration test will be useful.

@jeroenflvr
Copy link
Author

Valid point. Updating.

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