Chore: Address minor nitpicks and more from code review by CodeRabbit

Author: amotl

cratedb_toolkit/cluster/cli.py

@@ -104,16 +104,15 @@ def ping(ctx: click.Context, cluster_id: str, cluster_name: str):
     Ping cluster: API and database.
     """
     with handle_command_errors("ping cluster"):
-        cluster_info = ClusterInformation.from_id_or_name(cluster_id=cluster_id, cluster_name=cluster_name)
         cluster = ManagedCluster(cluster_id=cluster_id, cluster_name=cluster_name).start()
         has_db_result = bool(cluster.query("SELECT 42 AS availability_check  -- ctk"))
         response = {
-            "meta": cluster_info.meta,
-            "cloud": cluster_info.ready,
+            "meta": cluster.info.meta,
+            "cloud": cluster.info.ready,
             "database": has_db_result,
         }
         jd(response)
-        sys.exit(0 if (cluster_info.ready and has_db_result) else 1)
+        sys.exit(0 if (cluster.info.ready and has_db_result) else 1)
 
 
 @make_command(cli, name="start", help=help_start)

cratedb_toolkit/cluster/core.py

@@ -123,14 +123,14 @@ def __init__(
         self.cluster_name = cluster_name
         self.settings = settings or ManagedClusterSettings()
         self.address = address
-        self.info: ClusterInformation = info or ClusterInformation()
+        self._info: t.Optional[ClusterInformation] = info
         self.stop_on_exit = stop_on_exit
         self.exists: bool = False
 
         # Default settings and sanity checks.
         self.cluster_id = self.cluster_id or self.settings.cluster_id or None
         self.cluster_name = self.cluster_name or self.settings.cluster_name or None
-        if self.cluster_id is None and self.cluster_name is None:
+        if not self.cluster_id and not self.cluster_name:
             raise DatabaseAddressMissingError(
                 "Failed to address cluster: Either cluster identifier or name needs to be specified"
             )
@@ -203,12 +203,22 @@ def delete(self) -> "ManagedCluster":
         raise NotImplementedError("Deleting cluster not implemented yet")
         return self
 
-    def probe(self) -> "ManagedCluster":
+    @property
+    def info(self) -> ClusterInformation:
+        if self._info is None:
+            raise ValueError("Cluster information not yet available")
+        return self._info
+
+    def probe(self, refresh: bool = False) -> "ManagedCluster":
         """
         Probe a CrateDB Cloud cluster, API-wise.
         """
+        if refresh:
+            self._info = None
+        if self._info is not None:
+            return self
         try:
-            self.info = ClusterInformation.from_id_or_name(cluster_id=self.cluster_id, cluster_name=self.cluster_name)
+            self._info = ClusterInformation.from_id_or_name(cluster_id=self.cluster_id, cluster_name=self.cluster_name)
             self.cluster_id = self.info.cloud["id"]
             self.cluster_name = self.info.cloud["name"]
             self.address = DatabaseAddress.from_http_uri(self.info.cloud["url"])

cratedb_toolkit/cluster/croud.py

@@ -327,7 +327,7 @@ def set_suspended(self, body: t.Dict[str, t.Any]):
         if data is None:
             if not errors:
                 errors = "Unknown error"
-            raise CroudException(f"Resuming cluster failed: {errors}")
+            raise CroudException(f"Suspending or resuming cluster failed: {errors}")
         _wait_for_completed_operation(
             client=self.client,
             cluster_id=self.cluster_id,

cratedb_toolkit/cluster/model.py

@@ -101,6 +101,11 @@ def meta(self):
 
     @property
     def health(self):
+        try:
+            self.acquire_dynamic_content()
+            pass
+        except Exception as e:
+            logger.warning(f"Failed to refresh cluster information: {e}")
         info = self.asdict()
         return {
             "meta": self.meta,
@@ -128,13 +133,9 @@ def asdict(self) -> t.Dict[str, t.Any]:
         """
         Convert to dictionary after acquiring dynamic content.
         """
-        try:
-            self.refresh()
-        except Exception as e:
-            logger.warning(f"Failed to refresh cluster information: {e}")
         return deepcopy(dataclasses.asdict(self))
 
-    def refresh(self):
+    def acquire_dynamic_content(self):
         """
         Refresh dynamic content. Here: The `database` attribute, after inquiring the cluster.
         """

cratedb_toolkit/io/croud.py

@@ -77,6 +77,7 @@ def fix_job_info_table_name(self):
         full-qualified representation.
 
         FIXME: Remove after upstream has fixed the flaw.
+               https://github.com/crate/croud/issues/566
         """
         job_info = self.info
         if "destination" in job_info and "table" in job_info["destination"]:

cratedb_toolkit/model.py

@@ -64,7 +64,9 @@ def from_string(cls, url: str) -> "DatabaseAddress":
         """
         Factory method to create an instance from an SQLAlchemy database URL in string format.
         """
-        if url.startswith("crate"):
+        # Parse the URL to determine the scheme.
+        parsed_url = URL(url)
+        if parsed_url.scheme == "crate":
             return cls.from_sqlalchemy_uri(url)
         else:
             return cls.from_http_uri(url)

cratedb_toolkit/shell/cli.py

@@ -75,7 +75,7 @@ def cli(
         cluster_id=cluster_id,
         cluster_name=cluster_name,
         cluster_url=cluster_url,
-    ).probe()
+    )
 
     if cluster.address is None:
         raise click.UsageError("Inquiring cluster address failed.")

doc/backlog/main.md

@@ -1,9 +1,22 @@
 # Main Backlog
 
 ## Iteration +0
-- CLI: Consider simplifying by using `--url` instead of `--sqlalchemy-url` or `--http-url`
+- CLI: Consider simplifying the API by using canonical `--cluster-url` instead of `--sqlalchemy-url` vs. `--http-url`
 - CLI: Explore consolidating all four addressing options into a single `--cluster` option
-  for more intuitive cluster addressing. This could be implemented as an option alongside existing methods.
+  for more intuitive cluster addressing. This could be implemented as a separate option alongside
+  existing methods.
+- `cratedb_toolkit/cfr/cli.py`: Migrate to `ctx.meta["address"]`, like `cratedb_toolkit/info/cli.py`
+- Cluster API: Currently, software tests do not prune tables on CrateDB Cloud.
+  This will probably cause a disk spillover sooner or later on this instance.
+- When running the test suite, a huge log obstructs the output information.
+  ```
+  2025-05-03 22:48:22,191 [crate.client.http                   ] DEBUG   : JSON response for stmt(SELECT * FROM "sys"."allocations")
+  2025-05-03 22:48:22,768 [crate.client.http                   ] DEBUG   : JSON response for stmt(SELECT * FROM "sys"."jobs_log")
+  ```
+- Refactor `model.py`, `util/database.py`, and possibly more, to `ctk.model` module namespace
+- Search:
+  - https://vectorvfs.readthedocs.io/en/latest/usage.html
+  - https://news.ycombinator.com/item?id=43896011
 
 ## Iteration +1
 - Table Loader: Refactor URL dispatcher, use fsspec
@@ -49,7 +62,7 @@
 - UX: Accept alias `--format {jsonl,ndjson}` for `--format json_row` 
 - Catch recursion errors:
   ```
-  CRATEDB_SQLALCHEMY_URL=crate://crate@localhost:4200/
+  CRATEDB_CLUSTER_URL=crate://crate@localhost:4200/
   ```
 - CLI: Verify exit codes.
 - UX: Rename `ctk cluster info` to `ctk status cluster --id=foo-bar-baz`

doc/cluster/backlog.md

@@ -12,19 +12,20 @@ a patch for some item, it will be very much welcome.
 [issues]: https://github.com/crate/cratedb-toolkit/issues
 
 ## Iteration +1
-- `ctk cluster info` does not include information about CrateDB yet.
-  => Connect to ex-WTF utilities.
-- Less verbosity by default for `ctk cluster` operations.
+- CLI: Less verbosity by default for `ctk cluster` operations
   Possibly display cluster operation and job execution outcomes, and `last_async_operation` details
-- Python API: Make `cluster.query` use the excellent `records` package
-- `ctk cluster list`: New
+- CLI: Implement `ctk cluster list`, `ctk cluster delete`
+- Managed: Use `keyring` for caching the JWT token, and compensate token expiry
 
 ## Iteration +2
+- Python API: Make `cluster.query` use the excellent `records` package
 - Xon.sh example
 - Windows support. Powershell?
 - Validate multi-cluster, multi-org operations work well and make sense
 - Implement JWT token expiry handling
 - Add Apache Spark / Databricks to the client bundle?
+- Use universal `DatabaseCluster` across the board. For that to happen, it will
+  need to gain an `DatabaseCluster.from_env()` entry point.
 
 ## Iteration +3
 - Complete UI: `ctk cluster health`
@@ -82,8 +83,16 @@ a patch for some item, it will be very much welcome.
              'running_operation': '',
              'status': 'GREEN'},
   ```
-- Shell: Currently it is not possible to create multiple instances of a `ManagedCluster`,
+- Shell: Currently it is not possible to create multiple instances of a `ManagedCluster`
   because parameters like `username` and `password` are obtained from the environment, 
   using different credentials for connecting to individual clusters.
 - Document `ManagedCluster`'s `stop_on_exit` option
 - `toolkit/cluster/cli.py`: Also respect cluster ID and name across the board of ex-WTF utilities.
+- `ctk cluster info` does not include information about CrateDB yet.
+  => Connect to ex-WTF utilities.
+- Make CLI options override ENV vars. Otherwise, using `--sqlalchemy-url=crate://`
+  will never work if an `.env` file is present.
+- SDK: Rename `ctk.cluster` to `ctk.sdk`? => No.
+- CLI: Shrink address URLs to single parameter `--cluster-url`
+- Changelog: Notify about breaking change with input address parameter names
+- Docs: Update guidelines about input address parameter preferences

doc/cluster/cli.md

@@ -1,4 +1,5 @@
 (cluster-api-cli)=
+
 # CrateDB Cluster CLI
 
 The `ctk cluster {start,info,stop}` subcommands provide higher level CLI