diff --git a/datadog_checks_dev/changelog.d/23584.fixed b/datadog_checks_dev/changelog.d/23584.fixed new file mode 100644 index 0000000000000..9a8e9d0c3c27e --- /dev/null +++ b/datadog_checks_dev/changelog.d/23584.fixed @@ -0,0 +1 @@ +Fixed Detection of Unpinned Dependencies for integrations-extras and marketplace repos diff --git a/datadog_checks_dev/datadog_checks/dev/tooling/commands/validate/dep.py b/datadog_checks_dev/datadog_checks/dev/tooling/commands/validate/dep.py index c44c36a4da4b8..b59063f67bebf 100644 --- a/datadog_checks_dev/datadog_checks/dev/tooling/commands/validate/dep.py +++ b/datadog_checks_dev/datadog_checks/dev/tooling/commands/validate/dep.py @@ -105,7 +105,7 @@ def verify_base_dependency(source, check_name, dependency, force_pinned=True, mi return not failed -def verify_dependency(source, name, python_versions, file): +def verify_dependency(source, name, python_versions, file, repo_core): for dependency_definitions in python_versions.values(): # Identify dependencies that are defined multiple times for the same set of environment markers requirements = [Requirement(dep) for dep in dependency_definitions] @@ -143,29 +143,30 @@ def verify_dependency(source, name, python_versions, file): return True - if not specifier_set: - message = f'Unpinned version found for dependency `{name}`: {format_check_usage(checks, source)}' - echo_failure(message) - annotate_error(file, message) - return False - elif len(specifier_set) > 1: - message = ( - f'Multiple unstable version pins `{specifier_set}` found for dependency `{name}` ' - f'(use a single == explicitly): {format_check_usage(checks, source)}' - ) - echo_failure(message) - annotate_error(file, message) - return False - - specifier = get_next(specifier_set) - if specifier.operator != '==': - message = ( - f'Unstable version pin `{specifier}` found for dependency `{name}` ' - f'(use == explicitly): {format_check_usage(checks, source)}' - ) - echo_failure(message) - annotate_error(file, message) - return False + if repo_core: + if not specifier_set: + message = f'Unpinned version found for dependency `{name}`: {format_check_usage(checks, source)}' + echo_failure(message) + annotate_error(file, message) + return False + elif len(specifier_set) > 1: + message = ( + f'Multiple unstable version pins `{specifier_set}` found for dependency `{name}` ' + f'(use a single == explicitly): {format_check_usage(checks, source)}' + ) + echo_failure(message) + annotate_error(file, message) + return False + + specifier = get_next(specifier_set) + if specifier.operator != '==': + message = ( + f'Unstable version pin `{specifier}` found for dependency `{name}` ' + f'(use == explicitly): {format_check_usage(checks, source)}' + ) + echo_failure(message) + annotate_error(file, message) + return False return True @@ -184,7 +185,7 @@ def dep(check, require_base_check_version, min_base_check_version): \b * Verify the uniqueness of dependency versions across all checks, or optionally a single check - * Verify all the dependencies are pinned. + * Verify all the dependencies are pinned when validating from integrations-core. * Verify the embedded Python environment defined in the base check and requirements listed in every integration are compatible. * Verify each check specifies a `CHECKS_BASE_REQ` variable for `datadog-checks-base` requirement @@ -197,6 +198,9 @@ def dep(check, require_base_check_version, min_base_check_version): agent_dependencies, agent_errors = read_agent_dependencies() agent_dependencies_file = get_agent_requirements() annotate_errors(agent_dependencies_file, agent_errors) + ctx = click.get_current_context() + repo_core = ctx.obj['repo_choice'] == 'core' + if agent_errors: for agent_error in agent_errors: echo_failure(agent_error) @@ -225,18 +229,17 @@ def dep(check, require_base_check_version, min_base_check_version): echo_failure(check_error) for name, versions in sorted(check_dependencies.items()): - if not verify_dependency('Checks', name, versions, req_source): - failed = True - - if name not in agent_dependencies: + if not verify_dependency('Checks', name, versions, req_source, repo_core): failed = True - message = ( - f'Dependency {name} found in the {check_name} integration requirements ' - 'but not in the agent requirements, run `ddev dep freeze` to sync them.' - ) - echo_failure(message) - annotate_error(req_source, message) - + if repo_core: + if name not in agent_dependencies: + failed = True + message = ( + f'Dependency {name} found in the {check_name} integration requirements ' + 'but not in the agent requirements, run `ddev dep freeze` to sync them.' + ) + echo_failure(message) + annotate_error(req_source, message) check_base_dependencies, check_base_errors = read_check_base_dependencies(checks) check_dependencies, check_errors = read_check_dependencies(checks) @@ -247,17 +250,18 @@ def dep(check, require_base_check_version, min_base_check_version): failed = True for name, python_versions in sorted(agent_dependencies.items()): - if not verify_dependency('Agent', name, python_versions, agent_dependencies_file): + if not verify_dependency('Agent', name, python_versions, agent_dependencies_file, repo_core): failed = True # Check that this dependency defined on the agent requirements is actually used # This only makes sense when we take all check dependencies into account - if check is None and name not in check_dependencies: - failed = True - message = f'Stale dependency needs to be removed by syncing: {name}' - echo_failure(message) - annotate_error(agent_dependencies_file, message) - continue + if repo_core: + if check is None and name not in check_dependencies: + failed = True + message = f'Stale dependency needs to be removed by syncing: {name}' + echo_failure(message) + annotate_error(agent_dependencies_file, message) + continue # Look for version mismatches for this dependency against individual checks agent_dependency_definitions = get_dependency_set(python_versions) diff --git a/ddev/tests/cli/validate/test_dep.py b/ddev/tests/cli/validate/test_dep.py index a4a223b5baed8..feaf577175b14 100644 --- a/ddev/tests/cli/validate/test_dep.py +++ b/ddev/tests/cli/validate/test_dep.py @@ -3,6 +3,8 @@ # Licensed under a 3-clause BSD style license (see LICENSE) import re +import pytest + from tests.helpers.api import write_file error_regex = re.compile(r"(?s)^\s*[A-Za-z0-9_\/.-]+\.toml has the following errors:\n(?: - .+\n)+") @@ -42,7 +44,8 @@ def test_invalid_third_party_integration(fake_repo, ddev): assert error_regex.search(result.output), f"Unexpected output: {result.output}" -def test_multiple_invalid_third_party_integrations(fake_repo, ddev): +@pytest.mark.parametrize('check_name', ['bad_check_2', 'bad_check_3']) +def test_multiple_invalid_third_party_integrations(fake_repo, ddev, check_name): write_file( fake_repo.path / 'bad_check_2', 'pyproject.toml', @@ -67,15 +70,13 @@ def test_multiple_invalid_third_party_integrations(fake_repo, ddev): """, ) - result = ddev('validate', 'dep', 'bad_check_2') - result_2 = ddev('validate', 'dep', 'bad_check_3') + result = ddev('validate', 'dep', check_name) assert result.exit_code == 1 assert error_regex.search(result.output), f"Unexpected output: {result.output}" - assert result_2.exit_code == 1 - assert error_regex.search(result_2.output), f"Unexpected output: {result_2.output}" -def test_one_valid_one_invalid_integration(fake_repo, ddev): +@pytest.mark.parametrize('check_name', ['valid_check_2', 'bad_check_4']) +def test_one_valid_one_invalid_integration(fake_repo, ddev, check_name): write_file( fake_repo.path / 'valid_check_2', 'pyproject.toml', @@ -99,9 +100,114 @@ def test_one_valid_one_invalid_integration(fake_repo, ddev): """, ) - result = ddev('validate', 'dep', 'valid_check_2') - result_2 = ddev('validate', 'dep', 'bad_check_4') + result = ddev('validate', 'dep', check_name) + if check_name == 'valid_check_2': + assert result.exit_code == 0 + assert match_regex.match(result.output), f"Unexpected output: {result.output}" + else: + assert result.exit_code == 1 + assert error_regex.search(result.output), f"Unexpected output: {result.output}" + + +UNPINNED_OPT_CHECK = 'unpinned_opt_check' + +UNPINNED_OPTIONAL_DEP = 'acme-unpinned-lib' +PYPROJECT_UNPINNED_OPTIONAL = f""" + [project] + dependencies = [ + "datadog-checks-base>=37.21.0", + ] + + [project.optional-dependencies] + libs = [ + "{UNPINNED_OPTIONAL_DEP}", + ] + """ + + +def _write_git_dep_check(repo_path, check_name: str) -> None: + git_dep = 'sample_git_pkg @ git+https://github.com/pypa/pip.git@24.0' + write_file( + repo_path, + 'agent_requirements.in', + f"""datadog-checks-base==37.21.0 +{git_dep} +""", + ) + write_file( + repo_path / check_name, + 'pyproject.toml', + f""" + [project] + dependencies = [ + "datadog-checks-base>=37.21.0", + ] + + [project.optional-dependencies] + libs = [ + "{git_dep}", + ] + """, + ) + + +def test_core_rejects_unpinned_optional_dependency(fake_repo, ddev): + """With `-c`, unpinned PyPI deps under optional-dependencies fail validation.""" + write_file( + fake_repo.path, + 'agent_requirements.in', + f"""datadog-checks-base==37.21.0 +{UNPINNED_OPTIONAL_DEP}==1.0.0 +""", + ) + write_file(fake_repo.path, f'{UNPINNED_OPT_CHECK}/pyproject.toml', PYPROJECT_UNPINNED_OPTIONAL) + result = ddev('-c', 'validate', 'dep', UNPINNED_OPT_CHECK) + assert result.exit_code == 1 + assert 'Unpinned version' in result.output + + +def test_core_rejects_unpinned_optional_dependency_default_repo(fake_repo, ddev): + """Default repo is core: unpinned optional PyPI deps fail without passing `-c`.""" + write_file( + fake_repo.path, + 'agent_requirements.in', + f"""datadog-checks-base==37.21.0 +{UNPINNED_OPTIONAL_DEP}==1.0.0 +""", + ) + write_file(fake_repo.path, f'{UNPINNED_OPT_CHECK}/pyproject.toml', PYPROJECT_UNPINNED_OPTIONAL) + result = ddev('validate', 'dep', UNPINNED_OPT_CHECK) + assert result.exit_code == 1 + assert 'Unpinned version' in result.output + + +@pytest.mark.parametrize( + 'repo_fixture, flag', + [ + pytest.param('fake_extras_repo', '-e', id='extras'), + pytest.param('fake_marketplace_repo', '-m', id='marketplace'), + ], +) +def test_non_core_repo_allows_unpinned_optional_dependency(repo_fixture, flag, ddev, request): + """Non-core repos allow unpinned optional PyPI deps (no integrations-core pin rules).""" + repo = request.getfixturevalue(repo_fixture) + write_file( + repo.path, + 'agent_requirements.in', + """datadog-checks-base>=37.21.0 +""", + ) + write_file(repo.path, f'{UNPINNED_OPT_CHECK}/pyproject.toml', PYPROJECT_UNPINNED_OPTIONAL) + result = ddev(flag, 'validate', 'dep', UNPINNED_OPT_CHECK) + assert result.exit_code == 0, result.output + assert 'Unpinned version' not in result.output + assert match_regex.match(result.output), f"Unexpected output: {result.output}" + + +def test_validate_dep_git_url_succeeds_on_core(fake_repo, ddev): + """Git URL deps return early from verify_dependency and are not treated as unpinned PyPI.""" + _write_git_dep_check(fake_repo.path, 'git_dep_check') + result = ddev('-c', 'validate', 'dep', 'git_dep_check') assert result.exit_code == 0 + assert 'Unpinned version' not in result.output assert match_regex.match(result.output), f"Unexpected output: {result.output}" - assert result_2.exit_code == 1 - assert error_regex.search(result_2.output), f"Unexpected output: {result_2.output}"