-
Notifications
You must be signed in to change notification settings - Fork 0
#271: Implemented checking SCS content for completeness and correctness #272
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
Changes from 1 commit
1b78d72
56c96bc
fe55bd6
ab87d18
ffe4373
3593c44
867bf70
65a1ec8
1665a3d
ed7c728
ac6fc17
496492a
22f4f99
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,9 @@ | |
get_option_set, | ||
get_scs, | ||
) | ||
from exasol.nb_connector.connections import open_pyexasol_connection | ||
from exasol.nb_connector.itde_manager import bring_itde_up | ||
from exasol.nb_connector.secret_store import Secrets | ||
|
||
LOG = logging.getLogger(__name__) | ||
|
||
|
@@ -63,6 +66,35 @@ def save( | |
return 0 | ||
|
||
|
||
def verify_connection(scs: Secrets) -> int: | ||
if BackendSelector(scs).use_itde: | ||
# Question: Is it OK, to let bring_itde_up modify the SCS content, here? | ||
False and bring_itde_up(scs) | ||
report.warning(f"Bring up ITDE currently disabled") | ||
return 1 | ||
try: | ||
open_pyexasol_connection(scs).execute("SELECT 1 FROM DUAL").fetchone() | ||
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. Should 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. Yes 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.
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. I will add verification for bucket FS parameters:
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. I moved the BucketFS verification to a separate ticket/PR: |
||
except Exception as ex: | ||
report.error(f"Failed to connect to the configured database {ex}") | ||
return 1 | ||
report.success("Connection to the configured database instance was successful.") | ||
return 0 | ||
|
||
|
||
def check_scs(scs_file: Path, connect: bool) -> int: | ||
""" | ||
Check the SCS content for completeness. Infer the required keys from | ||
backend and use_itde if these are contained in the SCS already. | ||
|
||
If parameter `connect` is True then also verify if a connection to the | ||
configured Exasol database instance is successful. | ||
""" | ||
options = get_option_set(scs_file) | ||
if not options or not options.check(): | ||
return 1 | ||
return 0 if not connect else verify_connection(options.scs) | ||
|
||
|
||
def show_scs_content(scs_file: Path) -> int: | ||
""" | ||
If the SCS contains a proper backend selection, then show the SCS | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
Is it OK, to let
bring_itde_up()
modify the SCS content, here?Uh oh!
There was an error while loading. Please reload this page.
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.
Additional question:
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.
See my comment above.
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.
Also, we need to bring the ITDE up and take it down, in a
try .. finally
manner.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 updated the docstring.
But the question regarding the ITDE is still pending.
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 had the initial discussion, that actually the CLI does not interact too much with the ITDE.
Additionally, launching the ITDE takes quite some time and hence I am not sure whether a user really wants to verify launching and connecting to a Docker DB controlled by ITDE without any significant parameters needing to be configured and hence no significant parameters needing to be verified either.
Binging the ITDE up and take it down, in a
try ... finally
manner still requires some additional logic compared to the other DB instance variants and hence, I assume, we will need a test, which will again increase the size of this PR.All in all, I want to ask for confirmation if the benefits of this feature justify the additional effort.
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.
In my opinion, it doesn't really makes sense to test the connection to the ITDE. If we want to make sure the parameters are correct, we should make sure their format is checked while setting them.