-
Notifications
You must be signed in to change notification settings - Fork 11
feat(FIR-46249): Firebolt Core support #434
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
Conversation
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.
LGTM, one comment
|
||
# Parse URL | ||
try: | ||
parsed = urlparse(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.
nit: Might be a super rare case, but if we find any url path or parameters present let's log a warning saying it will be ignored.
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.
We're actually passing down everything right now, query parameters and path. IMO, it's up to the Core to filter those out. If the user has a server on top of the Core that takes in some parameters and based on them reroutes the request then they can work with the sdk out of the box.
Just checked and Core raises an error if extra query parameters are passed. However, it allows any path e.g. http://localhost:3473/my_awesome/path
returns the same results as http://localhost:3473/
|
Adding support for Firebolt Core.
Core does not have authentication, multiple accounts or engines.
Adding changes to the tests to run all the V2 DBAPI tests against core as well, by adding "core" filter.