[ddev-migration] Migrate create from datadog_checks_dev to ddev and refresh the command UX#23859
Open
AAraKKe wants to merge 13 commits into
Open
[ddev-migration] Migrate create from datadog_checks_dev to ddev and refresh the command UX#23859AAraKKe wants to merge 13 commits into
AAraKKe wants to merge 13 commits into
Conversation
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.
|
Contributor
|
|
…refix
`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`.
`_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.
… 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.
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.
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.
`_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.
…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.
`_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.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15b0382e89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+170
to
+174
| @click.group( | ||
| cls=_CreateGroup, | ||
| short_help='Scaffold a new integration', | ||
| context_settings={'help_option_names': ['-h', '--help']}, | ||
| ) |
There was a problem hiding this comment.
Because --type is only handled inside resolve_command, Click rejects it before this method runs when it appears before the positional command/name, e.g. the previously valid ddev create --type jmx MyJMXIntegration now fails with No such option: --type. The commit says --type is kept as a deprecation shim, and this is also the form that existed in the docs before this change, so the shim should accept options in the legacy prefix position as well as after the name.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Good catch, confirmed and fixed in 7f4080d. The group now sets ignore_unknown_options so the legacy --type / -t flag is accepted in the prefix position (ddev create --type jmx NAME, the pre-migration doc form) as well as after the name. Added regression tests for the --type X, --type=X, and -t X prefix spellings, plus a dropped-type prefix abort; subcommand option typos are still rejected since the setting doesn't propagate to the resolved subcommand.
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.
Contributor
Validation ReportAll 21 validations passed. Show details
|
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f4080d45b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import re | ||
| from datetime import datetime, timezone | ||
|
|
||
| VALID_INTEGRATION_NAME = re.compile(r'^[A-Z0-9](?:[A-Z0-9._\- ]*[A-Z0-9])?$', re.IGNORECASE) |
There was a problem hiding this comment.
When the user runs ddev create check 123foo (or uses a keyword such as class), this validator accepts the name because it only requires an alphanumeric first character. The scaffolder then uses the normalized name as both a Python package segment and the check class stem, producing invalid Python such as class 123fooCheck(...) or imports like from datadog_checks.class import ..., so the generated integration cannot even compile. Please reject names whose normalized package name is not identifier-safe and not a keyword, or derive safe Python identifiers separately.
Useful? React with 👍 / 👎.
Comment on lines
+172
to
+174
| 'author_name': author_normalized, | ||
| 'check_name': check_name, | ||
| 'email': (manifest.get('author') or {}).get('support_email'), |
There was a problem hiding this comment.
For ddev create check-only partner_thing with a manifest author, these prefilled values are stored as author_name and email, but the check-only templates interpolate {author} and {email_packages} in pyproject.toml. Because construct_template_fields() initializes those fields to empty strings for check_only, the generated package contains authors = [{ name = "", email = "" }] even though the manifest had the partner name and support email. Please map the manifest values to the fields the templates actually use.
Useful? React with 👍 / 👎.
luisorofino
reviewed
May 29, 2026
Contributor
There was a problem hiding this comment.
Question: can't we reuse the updated template we have in ddev/src/ddev/cli/create/templates/check/{check_name}/datadog_checks/{check_name}/check.py ? I think they should be the same, and now that the check one is modernized we should take advantage of that and also udpate this one.
Contributor
There was a problem hiding this comment.
Same for the test files too, right?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Migrates
createfromdatadog_checks_devinto a native ddev command (ddev/src/ddev/cli/create/) and refreshes its UX. The legacytooling/tree stays byte-identical to master (in-toto rule); the only change outsidecli/create/is the import swap incli/__init__.py.The command is now a click group with one subcommand per type, and manifest-less is the default:
ddev create {check,check-only,jmx,logs,event,metrics-crawler} NAME— one subcommand per supported type.--include-manifestrestores the legacymanifest.jsonoutput.--display-name,--metrics-prefix,--platformspopulate the three.ddev/config.tomloverrides a manifest-less integration needs. Supplied as flags → no prompts; missing in a TTY → prompt with a default; missing without a TTY → abort listing the missing flags.tile,snmp_tile, andmarketplaceare no longer exposed (replaced by PubPlatform). Their template dirs stay underdatadog_checks_dev/until the Phase 5 cleanup.--non-interactiveis removed;--skip-manifestand--typeare kept for one release as deprecation shims.Motivation
ddev createis the entry point the integration-agent automation drives, and the legacy command blocked that use:--skip-manifestonly skipped acheck_onlypre-flight while every other type still wrote amanifest.json, manifest-less integrations need three.ddev/config.tomloverrides that nothing automated, thetile/snmp_tiletypes are dead, and--non-interactivedid not actually prevent prompts when fields were missing. Porting and revamping in one PR gives the automation a single landing point instead of a pure port that is immediately replaced. Background: Building Integrations Without a Manifest.Template parity: templates are unchanged except one intentional JMX fix
The revamp changes the command code, not the templates. The six kept template trees were copied verbatim from
datadog_checks_dev/.../tooling/templates/integration/intoddev/src/ddev/cli/create/templates/. A standalone validator compares every template file in the new location against the same file onmaster(read withgit show) and fails on any unexpected difference, missing file, or new file.One file differs on purpose:
jmx/{check_name}/datadog_checks/{check_name}/config_models/defaults.py. The legacy template defined its default helpers with a(field, value)signature, but the generatedinstance.pycalls them with zero arguments — so every scaffolded JMX integration crashed at runtime. The fix is part of this PR and is the validator's one allow-listed exception.Result (run against the PR head):
How to run it (stdlib only; needs a
gitthat can seemaster). Save the script below ascheck_template_parity.pyat the repo root and runpython3 check_template_parity.py. It exits0when the only difference is the documented JMX fix,1otherwise.check_template_parity.py
Review checklist (to be filled by reviewers)
ddev/tests/cli/create/.qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is mergedThis PR has been created and validated using the paired-review skill from agent-integrations. Ready for human review.