Make validate all validate all available options#23228
Make validate all validate all available options#23228
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:
|
![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: f2190c91b2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (licenses, ('core',), 'repo', 'sync'), | ||
| (metadata, None, 'integrations', 'sync'), | ||
| (models, None, 'check', 'sync'), | ||
| (openmetrics, None, 'integrations', None), |
There was a problem hiding this comment.
VALIDATORS currently runs openmetrics for every repo (repos=None), but ddev/src/ddev/cli/validate/openmetrics.py aborts when the repo is not core or extras. In practice, ddev validate all on marketplace (and other unsupported repo contexts) will always record openmetrics as a failed validation even when nothing is wrong, which breaks the goal of mirroring CI behavior where this validator is only enabled when requested for compatible repos.
Useful? React with 👍 / 👎.
| VALIDATORS: tuple[tuple[click.BaseCommand, tuple[str, ...] | None, str, str | None], ...] = ( | ||
| (agent_reqs, ('core',), 'check', None), | ||
| (ci, None, 'repo', 'sync'), | ||
| (codeowners, ('extras',), 'repo', None), |
There was a problem hiding this comment.
The registry limits codeowners to extras, so ddev validate all skips this check in core. However, .github/workflows/validate.yml enables codeowners: true for repo: core, and the codeowners command contains core-specific validation (logs ownership), so this omission means local validate all can miss failures that CI will catch.
Useful? React with 👍 / 👎.
| (licenses, ('core',), 'repo', 'sync'), | ||
| (metadata, None, 'integrations', 'sync'), | ||
| (models, None, 'check', 'sync'), | ||
| (openmetrics, None, 'integrations', None), |
| (licenses, ('core',), 'repo', 'sync'), | ||
| (metadata, None, 'integrations', 'sync'), | ||
| (models, None, 'check', 'sync'), | ||
| (openmetrics, None, 'integrations', None), |
There was a problem hiding this comment.
Request: openmetrics is registered with applicable_repos=None (runs on all repos), but the validator internally calls app.abort() with exit code 1 when the repo is not core or extras. On marketplace or internal repos, ddev validate all will catch this as click.exceptions.Exit(1) and record openmetrics as a failed validation — a false positive.
Fix:
(openmetrics, ('core', 'extras'), 'integrations', None),
| VALIDATORS: tuple[tuple[click.BaseCommand, tuple[str, ...] | None, str, str | None], ...] = ( | ||
| (agent_reqs, ('core',), 'check', None), | ||
| (ci, None, 'repo', 'sync'), | ||
| (codeowners, ('extras',), 'repo', None), |
There was a problem hiding this comment.
Suggestion: codeowners is restricted to ('extras',), but CI runs codeowners: true for the core repo too. The validator performs a log-asset ownership check on core (line 118 of codeowners.py). This is a pre-existing gap inherited from the old ALL_VALIDATIONS, but since the PR's goal is to "mimic what CI does," consider widening to ('core', 'extras').
| if sync and fix_param is not None: | ||
| kwargs[fix_param] = True | ||
|
|
||
| try: |
There was a problem hiding this comment.
Suggestion: Only SystemExit and click.exceptions.Exit are caught. An unexpected exception (e.g., KeyError, FileNotFoundError) from any validator will halt the entire run, preventing remaining validators from executing and suppressing the failure summary. For an orchestrator whose job is running all validators, a broad except Exception with error logging would be more resilient:
except Exception as e:
failed.append(func.name)
app.display_error(f'{func.name} raised unexpected error: {e}')Praise: Catching both SystemExit (from legacy validators using sys.exit()) and click.exceptions.Exit (from ddev validators using app.abort()) with distinct truthiness checks is the right approach. Collecting failures rather than short-circuiting gives users a complete picture in one run.
| # 'integrations' — ddev validators that take an `integrations` tuple argument | ||
| # 'repo' — repo-level validators that take no check/integration argument | ||
| # fix_param: the keyword argument name for auto-fixing ('sync', 'fix'), or None if not supported | ||
| VALIDATORS: tuple[tuple[click.BaseCommand, tuple[str, ...] | None, str, str | None], ...] = ( |
There was a problem hiding this comment.
Suggestion: The VALIDATORS tuple uses positional fields with arg_mode typed as bare str. A typo like 'integration' instead of 'integrations' silently falls through both branches and invokes the validator with no arguments. A NamedTuple with Literal['check', 'integrations', 'repo'] for arg_mode would make entries self-documenting and catch misconfigurations at type-check time:
from typing import Literal, NamedTuple
class ValidatorSpec(NamedTuple):
func: click.BaseCommand
repos: tuple[str, ...] | None
arg_mode: Literal['check', 'integrations', 'repo']
fix_param: str | None
| If `check` is specified, only the check will be validated, if check value is 'changed' will only apply to changed | ||
| checks, an 'all' or empty `check` value will validate all README files. | ||
| """ | ||
| warnings.warn( |
There was a problem hiding this comment.
Suggestion: DeprecationWarning is silently suppressed by default in non-test, non--Wd Python environments. CLI users invoking the legacy path will see no deprecation message. Since this is a CLI command (not a library API), a visible click.echo warning or FutureWarning (which is shown by default) would actually reach the user.
| return {func.name for func, _, _, _ in VALIDATORS} | ||
|
|
||
|
|
||
| def test_validators_match_ci_workflow(): |
There was a problem hiding this comment.
Suggestion: The single test validates name-set parity between the VALIDATORS registry and the CI workflow — a valuable contract check. But the core behaviors introduced by this PR are untested: repo filtering, --sync flag propagation, and failure collection. The repo already has ddev fixture infrastructure and fake_repo/fake_marketplace_repo helpers that would enable integration-level tests. At minimum, a test verifying that repo-restricted validators are skipped on non-matching repos would catch regressions.
| ) | ||
| @click.pass_context | ||
| def all(ctx: click.Context, check: str | None, sync: bool) -> None: | ||
| """Run all CI validations for a repo. |
There was a problem hiding this comment.
Nit: The docstring says "If check is specified, only that check will be validated" — but this isn't true for 'repo'-mode validators (ci, dep, labeler, licenses, codeowners), which always run repo-wide regardless of the check argument. Also, "An empty or 'all' value" is slightly misleading since check defaults to None, not an empty string.
|
|
||
| from ddev.cli.validate.all import VALIDATORS | ||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parents[4] |
There was a problem hiding this comment.
Nit: REPO_ROOT = Path(__file__).resolve().parents[4] hardcodes the assumption the test lives exactly 4 levels below the repo root. If the file is ever relocated, this silently resolves to the wrong directory. An assertion guard like assert REPO_ROOT.joinpath('.github').is_dir() would make failures self-explanatory.
steveny91
left a comment
steveny91
left a comment
There was a problem hiding this comment.
Hi @steveny91, this is a first quick review powered by Claude Code. This uses a team of agents reviewing different factors of the code changes such as code quality, functionality, correctness and other aspects I find important. Rules are tailored following my personal recommendations and the review has been first approved by me.
Please take a look at the comments and decide whether they should be implemented or not. When deciding not to implement a comment make sure to say why, I will be reviewing both the code and your comments personally. This is a first iteration trying to catch the most important things.
My Feedback Legend
Here is a quick guide to the prefixes I use in my comments:
praise: no action needed, just celebrate!
note: just a comment/information, no need to take any action.
question: I need clarification or I am seeking to understand your approach.
nit: A minor, non-blocking issue (e.g., style, typo). Feel free to ignore.
suggestion: I am proposing an improvement. This is optional but recommended.
request: A change I believe is necessary before this can be merged.
The only blocking comments are request, any other type of comment can be applied at discretion of the developer.
| (licenses, ('core',), 'repo', 'sync'), | ||
| (metadata, None, 'integrations', 'sync'), | ||
| (models, None, 'check', 'sync'), | ||
| (openmetrics, None, 'integrations', None), |
There was a problem hiding this comment.
Request: openmetrics is registered with applicable_repos=None (runs on all repos), but the validator internally calls app.abort() with exit code 1 when the repo is not core or extras. On marketplace or internal repos, ddev validate all will catch this as click.exceptions.Exit(1) and record openmetrics as a failed validation — a false positive.
Fix:
(openmetrics, ('core', 'extras'), 'integrations', None),| VALIDATORS: tuple[tuple[click.BaseCommand, tuple[str, ...] | None, str, str | None], ...] = ( | ||
| (agent_reqs, ('core',), 'check', None), | ||
| (ci, None, 'repo', 'sync'), | ||
| (codeowners, ('extras',), 'repo', None), |
There was a problem hiding this comment.
Suggestion: codeowners is restricted to ('extras',), but CI runs codeowners: true for the core repo too. The validator performs a log-asset ownership check on core (line 118 of codeowners.py). This is a pre-existing gap inherited from the old ALL_VALIDATIONS, but since the PR's goal is to "mimic what CI does," consider widening to ('core', 'extras').
| if sync and fix_param is not None: | ||
| kwargs[fix_param] = True | ||
|
|
||
| try: |
There was a problem hiding this comment.
Suggestion: Only SystemExit and click.exceptions.Exit are caught. An unexpected exception (e.g., KeyError, FileNotFoundError) from any validator will halt the entire run, preventing remaining validators from executing and suppressing the failure summary. For an orchestrator whose job is running all validators, a broad except Exception with error logging would be more resilient:
except Exception as e:
failed.append(func.name)
app.display_error(f'{func.name} raised unexpected error: {e}')Praise: Catching both SystemExit (from legacy validators using sys.exit()) and click.exceptions.Exit (from ddev validators using app.abort()) with distinct truthiness checks is the right approach. Collecting failures rather than short-circuiting gives users a complete picture in one run.
| # 'integrations' — ddev validators that take an `integrations` tuple argument | ||
| # 'repo' — repo-level validators that take no check/integration argument | ||
| # fix_param: the keyword argument name for auto-fixing ('sync', 'fix'), or None if not supported | ||
| VALIDATORS: tuple[tuple[click.BaseCommand, tuple[str, ...] | None, str, str | None], ...] = ( |
There was a problem hiding this comment.
Suggestion: The VALIDATORS tuple uses positional fields with arg_mode typed as bare str. A typo like 'integration' instead of 'integrations' silently falls through both branches and invokes the validator with no arguments. A NamedTuple with Literal['check', 'integrations', 'repo'] for arg_mode would make entries self-documenting and catch misconfigurations at type-check time:
from typing import Literal, NamedTuple
class ValidatorSpec(NamedTuple):
func: click.BaseCommand
repos: tuple[str, ...] | None
arg_mode: Literal['check', 'integrations', 'repo']
fix_param: str | None| If `check` is specified, only the check will be validated, if check value is 'changed' will only apply to changed | ||
| checks, an 'all' or empty `check` value will validate all README files. | ||
| """ | ||
| warnings.warn( |
There was a problem hiding this comment.
Suggestion: DeprecationWarning is silently suppressed by default in non-test, non--Wd Python environments. CLI users invoking the legacy path will see no deprecation message. Since this is a CLI command (not a library API), a visible click.echo warning or FutureWarning (which is shown by default) would actually reach the user.
| return {func.name for func, _, _, _ in VALIDATORS} | ||
|
|
||
|
|
||
| def test_validators_match_ci_workflow(): |
There was a problem hiding this comment.
Suggestion: The single test validates name-set parity between the VALIDATORS registry and the CI workflow — a valuable contract check. But the core behaviors introduced by this PR are untested: repo filtering, --sync flag propagation, and failure collection. The repo already has ddev fixture infrastructure and fake_repo/fake_marketplace_repo helpers that would enable integration-level tests. At minimum, a test verifying that repo-restricted validators are skipped on non-matching repos would catch regressions.
| ) | ||
| @click.pass_context | ||
| def all(ctx: click.Context, check: str | None, sync: bool) -> None: | ||
| """Run all CI validations for a repo. |
There was a problem hiding this comment.
Nit: The docstring says "If check is specified, only that check will be validated" — but this isn't true for 'repo'-mode validators (ci, dep, labeler, licenses, codeowners), which always run repo-wide regardless of the check argument. Also, "An empty or 'all' value" is slightly misleading since check defaults to None, not an empty string.
|
|
||
| from ddev.cli.validate.all import VALIDATORS | ||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parents[4] |
There was a problem hiding this comment.
Nit: REPO_ROOT = Path(__file__).resolve().parents[4] hardcodes the assumption the test lives exactly 4 levels below the repo root. If the file is ever relocated, this silently resolves to the wrong directory. An assertion guard like assert REPO_ROOT.joinpath('.github').is_dir() would make failures self-explanatory.
1 participant
What does this PR do?
The current validate all doesn't run all validation that we need in CI. This change ports over validate all to ddev and add additional validations to mimic what CI does. Also introduces --sync to it.
e.g.: