-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Fir 12126 make engine parameter optional #152
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
Merged
stepansergeevitch
merged 8 commits into
main
from
FIR-12126-make-engine-parameter-optional
Mar 31, 2022
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
221b69b
allow not passing engine to connection
stepansergeevitch 0d406c9
add unit tests
stepansergeevitch 069f31a
add integration tests
stepan-northspyre 98d49b8
remove debug
stepan-northspyre f5ca9a2
use single endpoint for default engine
stepan-northspyre 93b8372
revert redundant changes
stepan-northspyre 9207bad
remove redundant comment
stepan-northspyre 2669224
updated docstring
stepan-northspyre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,11 @@ | |
FireboltEngineError, | ||
InterfaceError, | ||
) | ||
from firebolt.common.urls import ACCOUNT_ENGINE_BY_NAME_URL, ACCOUNT_ENGINE_URL | ||
from firebolt.common.urls import ( | ||
ACCOUNT_ENGINE_BY_NAME_URL, | ||
ACCOUNT_ENGINE_URL, | ||
ACCOUNT_ENGINE_URL_BY_DATABASE_NAME, | ||
) | ||
from firebolt.common.util import fix_url_schema | ||
|
||
DEFAULT_TIMEOUT_SECONDS: int = 5 | ||
|
@@ -65,17 +69,43 @@ async def _resolve_engine_url( | |
raise InterfaceError(f"Unable to retrieve engine endpoint: {e}.") | ||
|
||
|
||
async def _get_database_default_engine_url( | ||
database: str, | ||
auth: AuthTypes, | ||
api_endpoint: str, | ||
account_name: Optional[str] = None, | ||
) -> str: | ||
async with AsyncClient( | ||
auth=auth, | ||
base_url=api_endpoint, | ||
account_name=account_name, | ||
api_endpoint=api_endpoint, | ||
) as client: | ||
try: | ||
account_id = await client.account_id | ||
response = await client.get( | ||
url=ACCOUNT_ENGINE_URL_BY_DATABASE_NAME.format(account_id=account_id), | ||
params={"database_name": database}, | ||
) | ||
response.raise_for_status() | ||
return response.json()["engine_url"] | ||
except ( | ||
JSONDecodeError, | ||
RequestError, | ||
RuntimeError, | ||
HTTPStatusError, | ||
KeyError, | ||
) as e: | ||
raise InterfaceError(f"Unable to retrieve default engine endpoint: {e}.") | ||
|
||
|
||
def _validate_engine_name_and_url( | ||
engine_name: Optional[str], engine_url: Optional[str] | ||
) -> None: | ||
if engine_name and engine_url: | ||
raise ConfigurationError( | ||
"Both engine_name and engine_url are provided. Provide only one to connect." | ||
) | ||
if not engine_name and not engine_url: | ||
raise ConfigurationError( | ||
"Neither engine_name nor engine_url is provided. Provide one to connect." | ||
) | ||
|
||
|
||
def _get_auth( | ||
|
@@ -120,7 +150,7 @@ async def connect_inner( | |
api_endpoint(optional): Firebolt API endpoint. Used for authentication. | ||
|
||
Note: | ||
Either `engine_name` or `engine_url` should be provided, but not both. | ||
Providing both `engine_name` and `engine_url` would result in an error. | ||
|
||
""" | ||
# These parameters are optional in function signature | ||
|
@@ -136,7 +166,15 @@ async def connect_inner( | |
# Mypy checks, this should never happen | ||
assert database is not None | ||
|
||
if engine_name: | ||
if not engine_name and not engine_url: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change the documentation on L153 to reflect the changes |
||
engine_url = await _get_database_default_engine_url( | ||
database=database, | ||
auth=auth, | ||
account_name=account_name, | ||
api_endpoint=api_endpoint, | ||
) | ||
|
||
elif engine_name: | ||
engine_url = await _resolve_engine_url( | ||
engine_name=engine_name, | ||
auth=auth, | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 if the real error is wrong credentials or expired/incorrect token?
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.
Sure, we could also catch
AuthenticationError
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.
On the other side, I think
AuthenticationError
is pretty representative itself, so I think we should just let it propagate further