#271: Implemented checking SCS content for completeness and correctness #272

ckunki · 2025-10-01T13:28:02Z

Closes #271

ckunki · 2025-10-01T13:29:55Z

exasol/nb_connector/cli/processing/processing.py


+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?


Is it OK, to let bring_itde_up() modify the SCS content, here?

Additional question:

Should the command line really launch the IDTE only for verifying the connection parameters?

As actually the ITDE does not require actual connection parameters but only offers optional(!) configuration of disk and mem size.

See my comment above.

Also, we need to bring the ITDE up and take it down, in a try .. finally manner.

I updated the docstring.
But the question regarding the ITDE is still pending.

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.

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.

ahsimb · 2025-10-02T10:19:40Z

exasol/nb_connector/cli/processing/option_set.py

+    def check(self):
        """
        Check if the content of the SCS is complete wrt. the selected
        backend as the required options depend on the selected backend.


Please say that the function raises the ScsCliError if the check fails. I have to look at the code to figure this out.

Thanks.
Please see next push updating the docstring.

exasol/nb_connector/cli/processing/option_set.py

ahsimb · 2025-10-02T10:22:51Z

exasol/nb_connector/cli/processing/option_set.py

+def get_option_set(scs_file: Path) -> OptionSet:
    """
    Return an instance of an OptionSet if the SCS contains a proper
    backend selection. Otherwise report an error and return None.


Please update the docstring according to your latest changes.

See next push.

ahsimb · 2025-10-02T10:32:15Z

exasol/nb_connector/cli/commands/check.py

 @add_params(SCS_OPTIONS)
 @click.option(
    "--connect/--no-connect",
    is_flag=True,


Can we make the default depend on the backend?

For a DB that is already running it would be a good idea to check that the configuration works. However, for the Docker DB it's a different story. On one hand, starting it up takes time. On the other hand, all you need to check is that the memory parameters are with reasonable range. If we can't start the ITDE it must be for some unrelated reason.

I very much agree on your statements.
So what should we do then?
Not verify Docker DB?

I would make the default = False for the DockerDB and True otherwise. Maybe slightly awkward to implement, but not too difficult.

Unfortunately, it seems to be not that easy.

The check CLI command is common for all DB instance variants. Hence, the command does not know up-front which backend to check but reads the backend from the SCS, instead.
That means that the default value for the CLI option --connect needs to be detected dynamically, not during the CLI configuration and definition of the CLI options, but later during runtime.

This has the effect, that we cannot use a simple boolean CLI option here, but we need a three-state flag

DEFAULT, i.e. TRUE for SaaS and on-premise, FALSE for ITDE / Docker DB.

TRUE (force connect)

FALSE (never verify connect)

This in turn requires adding an Enum class and I think we should also add dedicated tests.

It also invalidates the current CLI option --connect/--no-connect using Click's is_flag=True feature.

Hence, we need to specify two separate Options operating on the same three-state flag. And each CLI option requires a separate help string.

Additionally, I think the default depending on the DB instance variant is also harder to explain to the user.

All this increases the size of the current PR by more than a few lines.

Again, if I am not wrong, I would like to ask for confirmation if the benefits of this feature justify the required effort.

ckunki · 2025-10-06T09:53:09Z

exasol/nb_connector/cli/processing/processing.py

+        report.warning(f"Bring up ITDE currently disabled")
+        return
+    try:
+        open_pyexasol_connection(scs).execute("SELECT 1 FROM DUAL").fetchone()


Should verify_connection() (CLI command scs check --connect) verify the bucketfs, too?

Is it sufficient to list the BFS contents in the test?

What is the expectation?

Are there any default files we expect to be contained in the BFS?

Or is it acceptable to modify the BFS contents for the sake of testing the connection?

Should the test also do a cleanup after having verified the connection?

I will add verification for bucket FS parameters:

list contents

choose unique file name

upload and download sample file

verify identical content

remove the file

I moved the BucketFS verification to a separate ticket/PR:

SCS CLI: Verify BucketFS Connection, too #274

exasol/nb_connector/cli/processing/option_set.py

tkilias · 2025-10-06T21:59:59Z

exasol/nb_connector/cli/processing/processing.py


+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?


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.

tkilias · 2025-10-06T22:00:13Z

exasol/nb_connector/cli/processing/processing.py

+        report.warning(f"Bring up ITDE currently disabled")
+        return
+    try:
+        open_pyexasol_connection(scs).execute("SELECT 1 FROM DUAL").fetchone()


tkilias · 2025-10-06T22:02:21Z

exasol/nb_connector/cli/processing/processing.py

+    try:
+        open_pyexasol_connection(scs).execute("SELECT 1 FROM DUAL").fetchone()
+    except Exception as ex:
+        raise ScsCliError(f"Failed to connect to the configured database {ex}")


Not sure, if only showing the exception message is helpful, maybe we should format the whole exception with stack trace

Please see next push for improved exception handling.

tkilias · 2025-10-06T22:04:10Z

exasol/nb_connector/cli/processing/processing.py

+    oset = get_option_set(scs_file)
+    for o in oset.options:


Not a fan of the short variable names

Please see next push, using a more expressive name.

Co-authored-by: Torsten Kilias <[email protected]>

tkilias · 2025-10-07T12:22:25Z

exasol/nb_connector/cli/processing/processing.py

    option_set = get_option_set(scs_file)
-    if not option_set:
-        return 1
    for o in option_set.options:


The name o is still not really meaning full

Fixed, please see latest push.

sonarqubecloud · 2025-10-07T14:53:39Z

Quality Gate failed

Failed conditions
1 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

#271: Implemented checking SCS content for completeness and correctness

1b78d72

ckunki requested a deployment to manual-approval October 1, 2025 13:28 — with GitHub Actions Waiting

ckunki commented Oct 1, 2025

View reviewed changes

Replaced old-school int return values by exceptions

56c96bc

ckunki requested a deployment to manual-approval October 1, 2025 14:55 — with GitHub Actions Waiting

ckunki requested a deployment to manual-approval October 1, 2025 15:00 — with GitHub Actions Waiting

project:fix

fe55bd6

ckunki requested a deployment to manual-approval October 1, 2025 15:02 — with GitHub Actions Waiting

formatted

ab87d18

ahsimb reviewed Oct 2, 2025

View reviewed changes

exasol/nb_connector/cli/processing/option_set.py Show resolved Hide resolved

ahsimb reviewed Oct 2, 2025

View reviewed changes

Removed dead code

ffe4373

ckunki requested a deployment to manual-approval October 2, 2025 12:50 — with GitHub Actions Waiting

Fixed review findings

3593c44

ckunki commented Oct 6, 2025

View reviewed changes

tkilias reviewed Oct 6, 2025

View reviewed changes

Apply suggestions from code review

867bf70

Co-authored-by: Torsten Kilias <[email protected]>

ckunki requested a deployment to manual-approval October 7, 2025 06:35 — with GitHub Actions Waiting

ckunki added 2 commits October 7, 2025 08:36

Cleanup regarding verify connection for Docker DB

65a1ec8

renamed oset to option_set

1665a3d

ckunki requested a deployment to manual-approval October 7, 2025 08:04 — with GitHub Actions Waiting

Added unit test for check --connect

ed7c728

ckunki requested a deployment to manual-approval October 7, 2025 08:57 — with GitHub Actions Waiting

ckunki added 2 commits October 7, 2025 10:58

Prepared verifying BucketFS connection

ac6fc17

Fixed exception raised in case of failure to connect

496492a

ckunki requested a deployment to manual-approval October 7, 2025 10:54 — with GitHub Actions Waiting

tkilias reviewed Oct 7, 2025

View reviewed changes

ckunki temporarily deployed to manual-approval October 7, 2025 13:47 — with GitHub Actions Inactive

Renamed variable "o" to "option"

22f4f99

tkilias approved these changes Oct 7, 2025

View reviewed changes

ckunki temporarily deployed to text-ai-prerelease October 7, 2025 14:24 — with GitHub Actions Inactive

ckunki merged commit cac24a3 into main Oct 7, 2025
26 of 27 checks passed

ckunki deleted the feature/271-Implemented_checking_SCS_content branch October 7, 2025 15:03

#271: Implemented checking SCS content for completeness and correctness #272

#271: Implemented checking SCS content for completeness and correctness #272

Uh oh!

Conversation

ckunki commented Oct 1, 2025

Uh oh!

ckunki Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckunki Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckunki Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsimb Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckunki Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 7, 2025

Quality Gate failed

Uh oh!

Uh oh!

Reviewers

Assignees

ckunki Oct 1, 2025 •

edited

Loading

ckunki Oct 1, 2025 •

edited

Loading

ckunki Oct 2, 2025 •

edited

Loading

ahsimb Oct 2, 2025 •

edited

Loading

ckunki Oct 7, 2025 •

edited

Loading