Migrate validate jmx-metrics from datadog_checks_dev to ddev#23652
Migrate validate jmx-metrics from datadog_checks_dev to ddev#23652AAraKKe wants to merge 5 commits into
Conversation
|
|
|
✨ Fix all issues with BitsAI or with Cursor
|
Codecov Report❌ Patch coverage is Additional details and impacted files🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05fde98e62
ℹ️ 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".
| selection: tuple[str, ...] = (check,) if check and check.lower() != 'all' else () | ||
| candidates = app.repo.integrations.iter(selection) |
There was a problem hiding this comment.
Pass the all sentinel for full validation
When CHECK is omitted or set to all, this converts the selection to (), but IntegrationRegistry.iter(()) is not an all-integrations selection in ddev: __finalize_selection treats an empty selection as the changed root entries (or returns None when there are no changes), while ('all',) is the sentinel that iterates every integration. As a result ddev validate jmx-metrics all and ddev validate jmx-metrics validate zero or only changed integrations instead of all JMX integrations, so most metrics files are skipped.
Useful? React with 👍 / 👎.
| if check and check.lower() == 'changed': | ||
| candidates = app.repo.integrations.iter_changed_code() |
There was a problem hiding this comment.
Include spec-only changes in changed validation
For the changed target this uses iter_changed_code(), which yields only integrations whose changed files satisfy Integration.requires_changelog_entry (package directory files or pyproject.toml). This validator also checks assets/configuration/spec.yaml, which is outside the package directory, so a PR that only edits a JMX integration's spec can remove the init_config/jmx or instances/jmx template and ddev validate all changed will not run jmx-metrics for that integration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actually this is funny becasue if we change the spec it also requires a changelog entry... I prefer to keep it like this for now and open a follow up pr to change that logic.
PR: #23655
…ly validate every JMX integration
An empty selection tuple makes IntegrationRegistry.__finalize_selection
fall back to its 'changed roots' path, which iterates zero (or only
changed) integrations instead of all of them. Use ('all',) so the
finalizer returns set(), producing the intended full iteration.
Add a parametrized regression test that creates three fake JMX checks
and asserts both 'ddev validate jmx-metrics' and
'ddev validate jmx-metrics all' validate all three.
Validation ReportAll 20 validations passed. Show details
|
What does this PR do?
Migrates the
validate jmx-metricscommand fromdatadog_checks_dev/.../tooling/commands/validate/jmx_metrics.pytoddev/src/ddev/cli/validate/jmx_metrics.py, rewriting the implementation to use ddev'sApplicationoutput style and ddev'sRepository/Integrationmodel. Adds a newIntegration.jmx_metrics_filecached property inddev/src/ddev/integration/core.pyso other commands can reuse the same file lookup. The legacy command file is deleted; entry removed fromALL_COMMANDSindatadog_checks_dev's validate group.Helper gap notes
tooling.utils.get_jmx_metrics_filehad no direct ddev equivalent. AddedIntegration.jmx_metrics_file(cached property returning aPath; caller checks.is_file()). PR 3.5 (meta jmx) is expected to reuse this property.Integration.is_jmx_checkalready existed; refactored to delegate tojmx_metrics_file.is_file()for consistency. Behavior unchanged.tooling.utils.is_jmx_integration(readsconf.yaml.exampleand looks upinit_config.is_jmx) is a different criterion fromIntegration.is_jmx_check(file presence). To preserve substantive parity with the legacy command, ported the conf-based check inline as a private_is_jmx_integrationhelper. ddev's nativeiter_jmx_checkswould have changed the matched set (e.g., would have includedhazelcast, which legacy excludes). Ifmeta jmxends up needing the same conf-based criterion, we can lift this helper to a shared module.tooling.utils.get_default_config_spec→ replaced withIntegration.config_spec.tooling.testing.process_checks_option→ replaced withapp.repo.integrations.iter(selection)plus a'changed'short-circuit toiter_changed_code().tooling.commands.console.annotate_error(GitHub Actions annotation): no ddev equivalent. Errors are still emitted viaapp.display_errorand surface via the non-zero exit code; CI annotations are dropped. (Tracked separately: anapp.annotate_*primitive will be added in a follow-up infrastructure PR; this command will be retrofitted then.)read_file/file_exists→Path.read_text()/.is_file()fromddev.utils.fs.Path.Test plan
ddev validate jmx-metrics --helpoutput identical to masterddev validate jmx-metrics tomcatproduces the same exit code (0) and output as masterddev validate jmx-metricsreturns the same set of 13 JMX integrations as legacyddev --no-interactive test ddevpasses (3 unrelated VCR-cassette failures from worktree-path-as-repo-name detection — being fixed separately in a parallel infrastructure PR)ddev --no-interactive test datadog_checks_devpasses (437/437)ddev/tests/cli/validate/test_jmx_metrics.pycover happy path, missing include, missing scope, duplicate beans, missing config spec, and missing JMX templates in specMotivation
Part of the
datadog_checks_dev→ddevCLI migration wave (PR 1.10). Every CLI command currently registered indatadog_checks_dev's legacy tooling tree is being moved into ddev as native code, with the legacytooling/directory deleted at the end of the multi-phase migration. This PR portsvalidate jmx-metricsand exposes aIntegration.jmx_metrics_fileproperty that downstream commands (notablymeta jmx) will reuse, advancing the wave's goal of consolidating CLI logic in ddev.Review checklist (to be filled by reviewers)
qa/skip-qalabel if the PR doesn't need to be tested during QA.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged