Feat: Add configurable discovery by ClassicMMT · Pull Request #18 · datamasque/datamasque-cli

ClassicMMT · 2026-07-17T03:13:19Z

No description provided.

ClassicMMT · 2026-07-17T03:17:37Z

Requires dm-python SDP PR and dm-python configurable discovery PR to be merged and dm-python==1.1.8 published on PyPI.

Note to self: once dm-python==1.1.8 is published, need to update uv.lock.

- Add support for datamasque-python 1.2.1 - Add a status command to rulesets, libraries, discover configs, and discover libraries - Refuse YAML of 60 KiB+ in rulesets/discover configs validate - Rework discover libraries for updated library model - Fix rulesets generate and connections update --password - Drop click dependency - Update changelog - Report server reason when library deletion is rejected

- Add validation to discover configs/libraries - Abort on empty YAML directly - Abort with not-found when a run has no discovery output

kw-datamasque · 2026-07-31T01:28:18Z

@@ -0,0 +1,328 @@
+"""Live-instance tests for configurable discovery."""


Nit:

Could we rename this file to test_discovery_configs.py, since (from what I see?) it tests discovery_configs.py and discovery_config_libraries.py but not discovery.py

After renaming, we will be able to remove this docstring since the file is self-explanatory.

kw-datamasque · 2026-07-31T01:29:55Z

+    named = [c for c in client.list_discovery_configs() if c.name == name]
+    matches = [c for c in named if c.config_type is expected_type]


Nit: Could we use explicit variable names: named_configs and matched_configs rather than named and matches - makes it easier to review and spot bugs.

kw-datamasque · 2026-07-31T03:24:02Z

+    Creates a temporary config to trigger server-side validation,
+    then deletes it. Reports any validation errors.
+
+    Note that configs over 60 KB validate asynchronously and cannot be validated here.


Everywhere else says KiB

kw-datamasque · 2026-07-31T03:40:52Z

+def _create_returning(
+    is_valid: ValidationStatus | None,
+    validation_errors: list[SimpleNamespace] | None = None,
+) -> Callable[[object], object]:
+
+    def fake_create(rs: object) -> object:
+        rs.id = 99  # type: ignore[attr-defined]
+        rs.is_valid = is_valid  # type: ignore[attr-defined]
+        rs.validation_errors = validation_errors or []  # type: ignore[attr-defined]
+        return rs
+
+    return fake_create


(Claude)

The name reads as "create returning ... what?" and Callable[[object], object] hides what actually flows through here, which is what forces the three # type: ignore[attr-defined] below.

The clone of this helper in test_discovery_configs.py is typed Callable[[DiscoveryConfig], DiscoveryConfig] and needs no ignores; typing this one against Ruleset drops all three.

A third copy lives in test_discovery_config_libraries.py, so the fake is spelled three ways across three files.

Suggestion: rename to _fake_create_with_validation, type it Callable[[Ruleset], Ruleset].

For bonus points, share one helper across the three test files.

kw-datamasque · 2026-07-31T03:42:04Z

+@patch(f"{MODULE}.get_client")
+def test_status_valid_exits_0(mock_get_client: MagicMock, runner: CliRunner) -> None:
+    client = MagicMock()
+    mock_get_client.return_value = client
+    client.list_rulesets.return_value = [_listed_ruleset(ValidationStatus.valid)]
+
+    result = runner.invoke(app, ["rulesets", "status", "demo", "--json"])
+
+    assert result.exit_code == 0
+    assert '"status": "valid"' in result.stdout


Can we use pytest.mark.parametrize for this and the 2 functions below?

kw-datamasque · 2026-08-02T19:43:04Z

+dm discover configs list [--type database|file]                    # List configs
+dm discover configs get <name> [--type database] [--yaml]          # Show details or raw YAML
+dm discover configs defaults [--type database|file] -o cfg.yaml    # Built-in default as a starting point
+dm discover configs create --name <n> --type database -f cfg.yaml  # Create/update from YAML
+dm discover configs delete <name> [--type database]                # Delete a config
+dm discover configs validate -f cfg.yaml --type database           # Validate against server (YAML under 60 KiB)
+dm discover configs status <name> [--type database]                # Validation status; poll after creating YAML of 60 KiB+


Should we be using [--type database|file] consistently?

kw-datamasque · 2026-08-02T19:45:19Z

+
+#### Discovery config libraries
+
+Libraries are untyped — the same library can be imported by both database and file discovery configs.


Suggested change

Libraries are untyped — the same library can be imported by both database and file discovery configs.

The same library can be imported by both database and file discovery configs.

kw-datamasque · 2026-08-02T19:54:30Z

    return str(match.id)


+def _resolve_discovery_config_id(


Within this function could we use reduce code by using DM-Python's get_discovery_config_by_name(), which finds discovery configs matching the given name and type?

kw-datamasque · 2026-08-02T20:02:31Z

+
+    OK = 0
+    ERROR = 1
+    USAGE = 2


I wasn't sure what USAGE means and why it was not part of EXIT_CODE_BY_ERROR.

It seems to be an error message emitted by Typer for command-line usage errors and is emitted before any code actually runs. Could we add a comment saying that please?

kw-datamasque · 2026-08-02T20:07:02Z

+
+
+def abort_api_error(prefix: str, exc: DataMasqueApiError, *, conflict_hint: str | None = None) -> NoReturn:
+    """Abort with the admin server's own explanation of a failed request."""


Suggested change

"""Abort with the admin server's own explanation of a failed request."""

"""Abort with DataMasque's explanation of a failed request."""

kw-datamasque · 2026-08-02T21:26:45Z

I like this idea of abort_api_error() which shows DataMasque's errors instead letting Typer handle errors itself.

However it may be easy to forget to add it in specific CLI commands - claude has found a few above. Preferably we would use these new abort_...() functions consistently across all the commands.

Could we create a top-level safety net, does something like the following work? (claude)

  def main() -> None:
      try:
          app()
      except DataMasqueApiError as exc:
          abort_api_error("Request failed", exc)
      except DataMasqueTransportError as exc:
          abort(str(exc), code=ErrorCode.TRANSPORT_ERROR)

Apart from consistently using abort_...() family and the UTF-8 issue above, most of the other comments are minor and this will be a good addition to the DataMasque CLI

ClassicMMT force-pushed the add-configurable-discovery branch from 425fe39 to 43946b9 Compare July 17, 2026 04:14

ClassicMMT mentioned this pull request Jul 19, 2026

Feat: Add Configurable Discovery Validation Capabilities datamasque/datamasque-python#37

Merged

ClassicMMT self-assigned this Jul 19, 2026

ClassicMMT changed the title ~~Add configurable discovery~~ Feat: Add configurable discovery Jul 19, 2026

ClassicMMT force-pushed the add-configurable-discovery branch from 2ae01e0 to d69ef28 Compare July 19, 2026 23:37

ClassicMMT requested review from kanewilliams and kw-datamasque and removed request for kanewilliams July 20, 2026 19:54

ClassicMMT force-pushed the add-configurable-discovery branch 2 times, most recently from 5bb10f2 to 5acaca4 Compare July 30, 2026 21:43

ClassicMMT marked this pull request as ready for review July 30, 2026 21:45

kw-datamasque mentioned this pull request Jul 31, 2026

feat: Add --config, --idd and --preview options to dm discover schema #17

Closed

ClassicMMT added 6 commits July 31, 2026 13:55

chore: Update to datamasque-python==1.2.2

4b45721

feat: Add support for configurable discovery

5a0d608

feat: add safe data preview support

eef74a5

style: Move error codes into an enum

79c0e3c

refactor: from dm-python

fdb5852

- Add validation to discover configs/libraries - Abort on empty YAML directly - Abort with not-found when a run has no discovery output

ClassicMMT force-pushed the add-configurable-discovery branch from 67b4915 to fdb5852 Compare July 31, 2026 01:55

kw-datamasque requested changes Aug 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feat: Add configurable discovery - #18

Feat: Add configurable discovery#18
ClassicMMT wants to merge 6 commits into
mainfrom
add-configurable-discovery

ClassicMMT commented Jul 17, 2026

Uh oh!

ClassicMMT commented Jul 17, 2026 •

edited

Loading

Uh oh!

kw-datamasque Jul 31, 2026

Uh oh!

kw-datamasque Jul 31, 2026

Uh oh!

kw-datamasque Jul 31, 2026

Uh oh!

kw-datamasque Jul 31, 2026

Uh oh!

kw-datamasque Jul 31, 2026

Uh oh!

kw-datamasque Aug 2, 2026

Uh oh!

kw-datamasque Aug 2, 2026

Uh oh!

kw-datamasque Aug 2, 2026

Uh oh!

kw-datamasque Aug 2, 2026

Uh oh!

kw-datamasque Aug 2, 2026

Uh oh!

kw-datamasque commented Aug 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,328 @@
		"""Live-instance tests for configurable discovery."""

		named = [c for c in client.list_discovery_configs() if c.name == name]
		matches = [c for c in named if c.config_type is expected_type]


		#### Discovery config libraries

		Libraries are untyped — the same library can be imported by both database and file discovery configs.

	Libraries are untyped — the same library can be imported by both database and file discovery configs.
	The same library can be imported by both database and file discovery configs.



		def abort_api_error(prefix: str, exc: DataMasqueApiError, *, conflict_hint: str \| None = None) -> NoReturn:
		"""Abort with the admin server's own explanation of a failed request."""

	"""Abort with the admin server's own explanation of a failed request."""
	"""Abort with DataMasque's explanation of a failed request."""

Uh oh!

Conversation

ClassicMMT commented Jul 17, 2026

Uh oh!

ClassicMMT commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kw-datamasque commented Aug 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ClassicMMT commented Jul 17, 2026 •

edited

Loading