Skip to content

Commit 1577abb

Browse files
AAraKKeluisorofino
andauthored
[ddev-migration] Migrate create from datadog_checks_dev to ddev and refresh the command UX (DataDog#23859)
* Migrate `create` from datadog_checks_dev to ddev and revamp the UX Port the `create` command into ddev as a click group with one subcommand per integration type (`check`, `check-only`, `jmx`, `logs`, `event`, `metrics-crawler`). The `tile`, `snmp_tile`, and `marketplace` templates are not exposed as subcommands but their files stay on disk under datadog_checks_dev/ (in-toto rule). UX changes bundled into the migration: - Manifest-less is the default. `--include-manifest` opts into the legacy manifest-shipped path. - `--display-name`, `--metrics-prefix`, `--platforms` are first-class options. Missing values prompt in TTY and abort listing every missing flag in non-TTY. - `--non-interactive` removed. `--skip-manifest` accepted as a no-op with a deprecation warning; conflicts with `--include-manifest`. - `--type` accepted as a deprecation shim; aborts for dropped types pointing at the Building Integrations Without a Manifest Confluence page. - Manifest-less integrations get three entries written into `.ddev/config.toml` under `[overrides.display-name]`, `[overrides.metrics-prefix]`, and `[overrides.manifest.platforms]`. Templates are copied into ddev's package data; pytest, ruff, and mypy configs exclude the templates tree. * Add changelog * Fix check_only target directory when integration name has an author prefix `ddev create check-only partner_thing` was scaffolding into `root/thing/` instead of populating the existing `root/partner_thing/` directory that holds the manifest. The scaffold helper now distinguishes the on-disk integration directory (`target_integration_dir`) from the template ``{check_name}`` substitution (`check_name_override`). Other changes: - Replace `click.echo(..., err=True)` and `ctx.fail(...)` in the ``--type`` deprecation shim with `app.display_warning` / `app.abort` so the messages flow through ddev's color and verbosity layer. - Honour `app.interactive` (and therefore the global `--interactive/--no-interactive` flag) instead of probing `sys.stdin.isatty()` directly. - Surface a clear, subcommand-pointing error when a user runs the legacy `ddev create NAME` shape with no `--type`. - Update `docs/developer/tutorials/logs/http-crawler.md` to the new `ddev create logs ACME …` form. - Extract a `create_options` decorator factory in `_common.py`; the six per-type subcommand modules now share one option contract. - Wrap template rendering with template-path context on format errors, and replace `assert isinstance(...)` in `TemplateFile.write()` with an explicit runtime check. - On a per-file write failure, abort with a "wrote N/M files, remove `<dir>` and retry" message instead of leaving the user guessing. - Probe `.ddev/config.toml` readability before scaffolding and emit a hand-edit fallback if the post-scaffold override write fails. - Tighten the `--type` shim's short-flag matching to an allow-list of known integration types so future `-tXxx` flags can't be eaten. - Drop the unused `include_manifest` parameter from `_resolve_check_only_inputs`; flatten the dead-code guard in `run_subcommand`. - New tests cover the check_only directory regression, the manifest-less check_only overrides path, the bare-positional error, and the global `--no-interactive` flag's effect on `create`. * Fix directory connector in dry-run tree at depth >= 2 `_format_line` was using `PIPE_END if last or is_dir else PIPE_MIDDLE` for nodes at depth >= 2, so every directory rendered with `└──` regardless of its position among siblings. Drop the `is_dir` short-circuit so non-last directories use `├──` like every other non-last node. Other changes: - Restructure `run_subcommand` into two explicit paths (manifest-less vs `--include-manifest`) so the manifest-less locals' scope matches their use; the override write moves into its own helper. - Remove the unreferenced `CHECK_ONLY_LINKS` constant and its entry in `INTEGRATION_TYPE_LINKS`. The `check_only` branch in `construct_template_fields` never consulted the dict. - Distinguish "no `--type` flag" from "`--type` with no value" via a dedicated `_MissingTypeValue` sentinel; the latter now aborts with a targeted message naming the missing value. - Wrap the `manifest.json` read in `_resolve_check_only_inputs` with `try/except (OSError, json.JSONDecodeError)` and route through `app.abort` for parity with the surrounding aborts. - Add a `read_config` fixture that loads `.ddev/config.toml` and switch the test module from the gated `tomli` to stdlib `tomllib` (Python 3.13). Deduplicate the three tests that previously inlined the parse-and-assert dance. - New tests for the tree-connector regression and the `--type`-with- no-value abort. * Fix JMX template config_models so scaffolded integrations don't crash at runtime Every `defaults` function in the JMX template was defined `(field, value)` while `instance.py` invoked them with zero arguments via `getattr(defaults, ..., lambda: value)()`. Any integration scaffolded with `ddev create jmx` raised `TypeError: ...() missing 2 required positional arguments` on first run. Drop the parameters to match every production JMX integration in the repo (e.g. `activemq`). The legacy template under `datadog_checks_dev/` stays byte-identical per the in-toto rule. Other changes: - `_extract_legacy_type` now treats a flag-shaped token following `--type` / `-t` (e.g. `ddev create NAME --type --dry-run`) as the flag's value being missing, so the user gets the targeted "requires a value" message instead of `Unknown integration type '--dry-run'`. - `construct_template_fields` switches from `INTEGRATION_TYPE_LINKS[...]` to `.get(..., '')` so a not-yet-registered subcommand doesn't raise an unattributed `KeyError` during scaffolding. - Tighten `_write_manifestless_overrides`'s `integration_dir` from `Any` to `Path`, and introduce a `CheckOnlyPrefillFields` `TypedDict` for the manifest-prefilled fields produced by `prefill_check_only_fields`. - Guard `_resolve_check_only_inputs` against non-object JSON manifests with a clean `app.abort('... does not contain a JSON object')`. - Document the `TemplateFile` invariant (`binary=True` => `bytes`, `binary=False` => `str`) so the two `# type: ignore` lines read as intentional. - `_write_files_with_cleanup_hint` now branches by `integration_type`: for `check_only`, list the scaffolded files (so the user knows what to remove without touching their `manifest.json`); for everything else, the existing "Remove `<integration_dir>` and retry" message stands. - Drop the unreachable `check_name_override` parameter from `render()` and the corresponding third return value of `_resolve_check_only_inputs`. `prefill_check_only_fields` already populates `check_name`, making the guard dead code. - Update `docs/developer/tutorials/jmx/integration.md` to the new `ddev create jmx NAME ...` form to match the round-1 update for the logs tutorial. * Reword test docstrings to describe behaviour and fit the line limit Several test docstrings in ddev/tests/cli/create/test_create.py described their own review-history provenance instead of the behaviour they lock in, and one of them was 131 characters wide which tripped ruff E501 on CI. Reword every affected docstring so each test reads as a self-contained behavioural assertion and stays under 120 chars. * Restore JMX metrics.py template to match legacy ground truth The new copy of `jmx/{check_name}/tests/metrics.py` had been cosmetically reformatted by ruff (multi-line list/concat layout) during an early implementation pass — before the templates tree was added to the ruff format/lint excludes. The two forms are functionally identical, but the new template now scaffolded a visibly different metrics.py than legacy `ddev create jmx` would have produced. Revert the file to the master legacy bytes so the two are byte-identical again. The ruff configs in `ddev/pyproject.toml` (`[tool.ruff].exclude`, `[tool.ruff.format].exclude`) and the root `pyproject.toml` already list `src/ddev/cli/create/templates`. Verified with `ddev test --lint ddev`: 303 files already formatted, no rewrite of the restored metrics.py. * Reject empty author name when scaffolding check-only integrations `_resolve_check_only_inputs` checked `if author is None` against the value pulled from `manifest_data["author"]["name"]`. An empty string (or whitespace-only string) passed that guard and propagated through `prefill_check_only_fields` (which filtered the field out) and `construct_template_fields` (which fell back to `check_name = ''`). The rendered template paths then collapsed to forms like `/datadog_checks//__about__.py`, and `Path(root) / '/abs/path'` discards `root` on POSIX — scaffolded files would have been written to the filesystem root. Switch to a truthy check on the `.strip()`-ed value so empty and whitespace-only author names abort cleanly. Other changes: - Replace the 13-line `_MissingTypeValue` singleton class with a bare module-level `_MISSING_TYPE_VALUE = object()`. The only consumer uses `is`-comparison, not `isinstance`, so the class machinery added nothing. - Extract `_TYPE_FLAG_LITERALS` and `_TYPE_FLAG_EQUALS_PREFIXES` module constants shared by `_extract_legacy_type` and `_strip_type_flag` so future spellings only have to be added in one place. * Reject all-symbol author names and surface invalid integration names cleanly `_resolve_check_only_inputs` previously accepted any truthy author name after `.strip()`. An all-symbol input like "!@#$" passed the guard but then `normalize_display_name` collapsed it to "", and the downstream `removeprefix` was a no-op — leaving the rendered template paths with a stray leading underscore. Consolidate the normalization and the truthy check: normalize first, abort when the normalized form is empty. Same guard now covers empty, whitespace-only, and all-symbol author names. Other changes: - Pre-validate the integration `name` argument in `run_subcommand` via a shared `is_valid_integration_name` predicate in `_naming.py`. Names with leading/trailing non-alphanumerics or characters outside `[A-Za-z0-9._\- ]` now abort with a clear message instead of surfacing a raw `ValueError` from `normalize_project_name`. - Swap the bare `_MISSING_TYPE_VALUE = object()` sentinel for a one-member `Enum` (`_TypeFlagSentinel.MISSING`). Same runtime semantics (identity comparison), distinct nominal type so mypy can narrow `_extract_legacy_type`'s return value. - Drop the per-subcommand `-q`/`--quiet` flag from `create_options`. The root `ddev` group already exposes a count-based `--quiet`/`--verbose`; `render()` now reads `app.quiet` and routes the one-line headline through `app.display` (unconditional) so `ddev -q create ...` still prints "Created `<dir>`". - Extract a `fail_on_second_write` pytest fixture in `conftest.py` and drop the duplicated `monkeypatch.setattr(TemplateFile.write, ...)` setup from the two partial-write tests. * Fix package name collision when manifest author contains a hyphen `_resolve_check_only_inputs` stripped the author prefix from the target integration directory using `normalize_display_name`, which preserves hyphens. The directory name is built via `normalize_package_name`, which converts hyphens to underscores. A manifest with `"author": {"name": "My-Partner"}` paired with a directory named `my_partner_thing` produced an `author_normalized = "my-partner"` prefix that no longer matched the underscore form in the dir. The `removeprefix` silently did nothing, and `prefill_check_only_fields` then re-prefixed the (unstripped) name with the same author segment, yielding a doubled `my_partner_my_partner_thing` Python package path. Use `normalize_package_name(author_normalized)` so the prefix matches the directory's normalization. Other changes: - Reword `ddev/changelog.d/23859.added` to describe the genuinely new user-facing surface (subcommand-based interface plus the new options) instead of the migration-implementation detail; the migration narrative stays in `23859.changed`. - Add behaviour tests for `--location` with both absolute and relative target paths, plus an interactive-prompt test that exercises the empty-input default-acceptance branch of `_resolve_manifestless_inputs`. - Add a comment at `TemplateFile.read()` documenting that template rendering failures indicate a broken shipped template, not user error — the user's `name` is validated upstream by `is_valid_integration_name` before scaffolding starts. * Show TOML section headers in the create config-write-failure message The manual-fix instructions printed when `.ddev/config.toml` can't be written listed the override values without their `[overrides.*]` section headers: Rich parsed the bracketed headers as style tags and stripped them. Pass markup=False so the headers survive. Also hardens the surrounding scaffolding: - Treat `--type=` with an empty value as a missing type, not the type name ``. - Derive check_class from the final check_name and assert it is populated before scaffolding, so an empty name can't collapse paths to the filesystem root. - Reuse the caller's already-normalized author in prefill_check_only_fields instead of re-deriving it from the raw manifest. - Narrow the override-setter type hints. - Add tests for the malformed-config and config-write-failure abort paths. * Test that create rejects --type= with an empty value * Accept the legacy --type flag before the positional name The deprecation shim resolved --type inside the group's command resolution, which click only reaches after its option parser runs. When --type appeared before the name (the form the pre-migration docs used, e.g. `ddev create --type jmx NAME`), the parser rejected it with "No such option" before the shim could dispatch. Setting ignore_unknown_options on the group lets the flag reach the shim in any position; subcommand option typos are still rejected because that setting does not propagate to the resolved subcommand. --------- Co-authored-by: Luis Orofino <luis.orofino@datadoghq.com>
1 parent b7ff093 commit 1577abb

112 files changed

Lines changed: 3870 additions & 6 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

ddev/changelog.d/23859.added

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Add subcommand-based `ddev create` interface (`check`, `check-only`, `jmx`, `logs`, `event`, `metrics-crawler`) with `--display-name`, `--metrics-prefix`, `--platforms`, and `--include-manifest` options for manifest-less integrations.

ddev/changelog.d/23859.changed

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Refresh the `ddev create` UX: the command is now a click group with one subcommand per integration type (`check`, `check-only`, `jmx`, `logs`, `event`, `metrics-crawler`). Manifest-less is the new default; pass `--include-manifest` to keep generating a `manifest.json`. New per-subcommand options `--display-name`, `--metrics-prefix`, and `--platforms` populate `.ddev/config.toml` overrides. The `tile`, `snmp_tile`, and `marketplace` types are no longer exposed; `--non-interactive` is removed; `--skip-manifest` is accepted with a deprecation warning; `--type` is accepted as a deprecation shim that dispatches to the matching subcommand.

ddev/pyproject.toml

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,17 @@ version-file = "src/ddev/_version.py"
7474
[tool.hatch.build.targets.sdist]
7575
include = ["src"]
7676

77+
[tool.hatch.build.targets.wheel]
78+
packages = ["src/ddev"]
79+
artifacts = ["src/ddev/cli/create/templates"]
80+
7781
[tool.hatch.build.targets.binary]
7882
python-version = "3.13"
7983
scripts = ["ddev"]
8084

8185
[tool.pytest.ini_options]
8286
asyncio_mode = "auto"
87+
norecursedirs = ["src/ddev/cli/create/templates"]
8388

8489
# Keep Black configuration to generate models through validate
8590
# Switch to Ruff after it provides a Python API
@@ -92,7 +97,9 @@ extend-exclude = "src/ddev/_version.py"
9297

9398
[tool.ruff]
9499
extend = "../pyproject.toml"
95-
exclude = []
100+
exclude = [
101+
"src/ddev/cli/create/templates",
102+
]
96103
target-version = "py313"
97104
line-length = 120
98105

@@ -128,7 +135,8 @@ unfixable = [
128135
[tool.ruff.format]
129136
quote-style = "preserve"
130137
exclude = [
131-
"src/ddev/_version.py"
138+
"src/ddev/_version.py",
139+
"src/ddev/cli/create/templates",
132140
]
133141

134142
[tool.ruff.lint.isort]

ddev/src/ddev/cli/__init__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55

66
import click
77
import pluggy
8-
from datadog_checks.dev.tooling.commands.create import create
98
from datadog_checks.dev.tooling.commands.run import run
109

1110
from ddev._version import __version__
@@ -14,6 +13,7 @@
1413
from ddev.cli.ci import ci
1514
from ddev.cli.clean import clean
1615
from ddev.cli.config import config
16+
from ddev.cli.create import create
1717
from ddev.cli.dep import dep
1818
from ddev.cli.docs import docs
1919
from ddev.cli.env import env
Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
# (C) Datadog, Inc. 2026-present
2+
# All rights reserved
3+
# Licensed under a 3-clause BSD style license (see LICENSE)
4+
"""
5+
``ddev create`` command group.
6+
7+
This module is intentionally light at import time: only ``click`` and the
8+
subcommand modules are imported up-front. Heavy helpers (template walker,
9+
config-override writer, ``tomli_w``) load inside the command function
10+
bodies so ``ddev create --help`` stays fast.
11+
"""
12+
13+
from __future__ import annotations
14+
15+
import enum
16+
17+
import click
18+
19+
from ddev.cli.create.check import check
20+
from ddev.cli.create.check_only import check_only
21+
from ddev.cli.create.event import event
22+
from ddev.cli.create.jmx import jmx
23+
from ddev.cli.create.logs import logs
24+
from ddev.cli.create.metrics_crawler import metrics_crawler
25+
26+
CONFLUENCE_NO_MANIFEST_URL = 'https://datadoghq.atlassian.net/wiki/spaces/AI/pages/6248108729/'
27+
28+
LEGACY_TYPE_TO_SUBCOMMAND: dict[str, str] = {
29+
'check': 'check',
30+
'check_only': 'check-only',
31+
'jmx': 'jmx',
32+
'logs': 'logs',
33+
'event': 'event',
34+
'metrics_crawler': 'metrics-crawler',
35+
}
36+
37+
DROPPED_LEGACY_TYPES = {'tile', 'snmp_tile', 'marketplace'}
38+
39+
40+
class _CreateGroup(click.Group):
41+
"""Group that accepts ``ddev create NAME --type=...`` as a deprecation shim."""
42+
43+
def resolve_command( # type: ignore[override]
44+
self, ctx: click.Context, args: list[str]
45+
) -> tuple[str | None, click.Command | None, list[str]]:
46+
from ddev.cli.application import Application
47+
48+
if not args:
49+
return super().resolve_command(ctx, args)
50+
51+
first = args[0]
52+
if self.get_command(ctx, first) is not None:
53+
return super().resolve_command(ctx, args)
54+
55+
app: Application = ctx.obj
56+
legacy_type = _extract_legacy_type(args)
57+
58+
if legacy_type is _MISSING_TYPE_VALUE:
59+
app.abort('`--type` / `-t` requires a value (e.g. `--type=check`).')
60+
61+
# No `--type`: the user passed a bare positional (the legacy `ddev create NAME` shape
62+
# that is now ambiguous). Point them at the new subcommand surface before click's
63+
# default "No such command" message lands.
64+
if legacy_type is None:
65+
app.abort(
66+
f'`ddev create {first}` is no longer supported. '
67+
f'Use a subcommand: `ddev create check {first}`, `ddev create logs {first}`, '
68+
f'etc. Run `ddev create --help` to see the full list.'
69+
)
70+
71+
if legacy_type in DROPPED_LEGACY_TYPES:
72+
app.abort(
73+
f'`--type={legacy_type}` is no longer supported. '
74+
f'Use the manifest-less workflow described at {CONFLUENCE_NO_MANIFEST_URL}.'
75+
)
76+
77+
# mypy doesn't propagate `app.abort`'s NoReturn through the typed `ctx.obj`
78+
# assignment, so narrow explicitly here.
79+
assert isinstance(legacy_type, str)
80+
target = LEGACY_TYPE_TO_SUBCOMMAND.get(legacy_type)
81+
if target is None:
82+
app.abort(f'Unknown integration type: `{legacy_type}`.')
83+
84+
subcommand = self.get_command(ctx, target)
85+
if subcommand is None:
86+
app.abort(f'Internal error: subcommand `{target}` not registered.')
87+
assert subcommand is not None # for the type checker; abort above is NoReturn
88+
89+
app.display_warning(
90+
f'`--type={legacy_type}` is deprecated. '
91+
f'Use `ddev create {target} NAME` instead. The flag will be removed in a future release.'
92+
)
93+
94+
cleaned = _strip_type_flag(args)
95+
return subcommand.name, subcommand, cleaned
96+
97+
98+
class _TypeFlagSentinel(enum.Enum):
99+
"""Nominal sentinel type so mypy can narrow ``_extract_legacy_type``'s return value."""
100+
101+
MISSING = 'missing'
102+
103+
104+
# Sentinel: ``--type`` / ``-t`` was passed but no value followed (e.g. trailing ``--type``).
105+
_MISSING_TYPE_VALUE: _TypeFlagSentinel = _TypeFlagSentinel.MISSING
106+
107+
# Recognised spellings of the deprecated ``--type`` / ``-t`` flag.
108+
# Used by both ``_extract_legacy_type`` and ``_strip_type_flag``; update once if a
109+
# new spelling is ever added.
110+
_TYPE_FLAG_LITERALS: tuple[str, ...] = ('--type', '-t')
111+
_TYPE_FLAG_EQUALS_PREFIXES: tuple[str, ...] = ('--type=', '-t=')
112+
113+
114+
def _extract_legacy_type(args: list[str]) -> str | _TypeFlagSentinel | None:
115+
"""Return the `--type` / `-t` value from ``args``.
116+
117+
Distinguishes three outcomes:
118+
- ``None``: no `--type` / `-t` flag present at all.
119+
- ``_MISSING_TYPE_VALUE``: the flag is present but has no following value.
120+
- ``str``: the flag is present with a value.
121+
"""
122+
iterator = iter(args)
123+
for token in iterator:
124+
if token in _TYPE_FLAG_LITERALS:
125+
value = next(iterator, _MISSING_TYPE_VALUE)
126+
# If the next token is itself a flag (e.g. `--type --dry-run`), treat the
127+
# value as missing — the user clearly didn't intend to pass `--dry-run`
128+
# as the type name.
129+
if isinstance(value, str) and value.startswith('-'):
130+
return _MISSING_TYPE_VALUE
131+
return value
132+
for prefix in _TYPE_FLAG_EQUALS_PREFIXES:
133+
if token.startswith(prefix):
134+
value = token[len(prefix) :]
135+
# `--type=` with nothing after the equals sign is a missing value, not the
136+
# empty-string type name `''` (which would abort with a confusing message).
137+
return value if value else _MISSING_TYPE_VALUE
138+
if _is_concatenated_short_type(token):
139+
return token[2:]
140+
return None
141+
142+
143+
def _is_concatenated_short_type(token: str) -> bool:
144+
"""Match the legacy ``-tcheck`` short-flag form without swallowing future ``-tXxx`` options."""
145+
if not token.startswith('-t') or len(token) <= 2 or token.startswith('--'):
146+
return False
147+
candidate = token[2:]
148+
return candidate in LEGACY_TYPE_TO_SUBCOMMAND or candidate in DROPPED_LEGACY_TYPES
149+
150+
151+
def _strip_type_flag(args: list[str]) -> list[str]:
152+
"""Remove the legacy ``--type`` / ``-t`` flag from ``args`` (allow-listed forms only)."""
153+
cleaned: list[str] = []
154+
skip_next = False
155+
for token in args:
156+
if skip_next:
157+
skip_next = False
158+
continue
159+
if token in _TYPE_FLAG_LITERALS:
160+
skip_next = True
161+
continue
162+
if token.startswith(_TYPE_FLAG_EQUALS_PREFIXES):
163+
continue
164+
if _is_concatenated_short_type(token):
165+
continue
166+
cleaned.append(token)
167+
return cleaned
168+
169+
170+
@click.group(
171+
cls=_CreateGroup,
172+
short_help='Scaffold a new integration',
173+
# ignore_unknown_options lets the legacy `--type` / `-t` flag survive the group's
174+
# option parser when it appears before the positional name (e.g. the previously
175+
# documented `ddev create --type jmx NAME`). Without it, click rejects `--type`
176+
# with "No such option" before resolve_command's deprecation shim ever runs.
177+
context_settings={'help_option_names': ['-h', '--help'], 'ignore_unknown_options': True},
178+
)
179+
def create() -> None:
180+
"""Scaffold a new integration.
181+
182+
Use one of the subcommands (``check``, ``check-only``, ``jmx``, ``logs``,
183+
``event``, ``metrics-crawler``).
184+
185+
The ``--type`` / ``-t`` flag from the legacy CLI is accepted as a
186+
deprecation shim and will be removed in a future release.
187+
"""
188+
189+
190+
create.add_command(check)
191+
create.add_command(check_only)
192+
create.add_command(jmx)
193+
create.add_command(logs)
194+
create.add_command(event)
195+
create.add_command(metrics_crawler)

0 commit comments

Comments
 (0)