From a33be2292e7c23b3c85bb490bd944bd675e9765d Mon Sep 17 00:00:00 2001 From: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com> Date: Wed, 25 Oct 2023 16:34:02 +0200 Subject: [PATCH] Handle package-specifier spacing (#197) Use the spacing between package name and specifier. New: - Add `--verbose` option to update-deps. - Add `debug` input for CI - Check dependencies. Log sub-line input to update_file() in update_deps(). Update tests to include spacing issues. Add test for the --verbose flag. Add comments in test according to code review. Avoid `name3` in test since multiple version specifiers are not reliably supported. Follow issue #141 for updates. Explain change in `name2`. Co-authored-by: Anders Eklund --- .../ci_check_pyproject_dependencies.yml | 42 ++++++++- ci_cd/tasks/update_deps.py | 41 ++++++-- docs/hooks/update_pyproject.md | 1 + .../ci_check_pyproject_dependencies.md | 1 + tests/tasks/test_update_deps.py | 94 ++++++++++++++++--- 5 files changed, 149 insertions(+), 30 deletions(-) diff --git a/.github/workflows/ci_check_pyproject_dependencies.yml b/.github/workflows/ci_check_pyproject_dependencies.yml index 1313ce60..5b5866f9 100644 --- a/.github/workflows/ci_check_pyproject_dependencies.yml +++ b/.github/workflows/ci_check_pyproject_dependencies.yml @@ -51,6 +51,11 @@ on: required: false type: string default: "" + debug: + description: "Whether to run the workflow in debug mode, printing extra debug information." + required: false + type: boolean + default: false secrets: PAT: description: "A personal access token (PAT) with rights to update the `permanent_dependencies_branch`. This will fallback on `GITHUB_TOKEN`." @@ -75,18 +80,36 @@ jobs: - name: Install Python dependencies run: | - python -m pip install -U pip - pip install -U setuptools wheel - pip install .${{ inputs.install_extras }} - pip install git+https://github.com/SINTEF/ci-cd.git@v2.5.2 + if [ "${{ inputs.debug }}" == "true" ]; then + set -x + VERBOSE=-vvv + else + VERBOSE= + fi + + python -m pip install -U ${VERBOSE} pip + pip install -U ${VERBOSE} setuptools wheel + pip install ${VERBOSE} .${{ inputs.install_extras }} + pip install ${VERBOSE} git+https://github.com/SINTEF/ci-cd.git@v2.5.2 - name: Set up git user run: | + if [ "${{ inputs.debug }}" == "true" ]; then + set -x + fi + git config --global user.name "${{ inputs.git_username }}" git config --global user.email "${{ inputs.git_email }}" - name: Run ci-cd task run: | + if [ "${{ inputs.debug }}" == "true" ]; then + set -x + VERBOSE=--verbose + else + VERBOSE= + fi + if [ "${{ inputs.fail_fast }}" == "true" ]; then FAIL_FAST=--fail-fast else @@ -98,10 +121,15 @@ jobs: if [ -n "${line}" ]; then IGNORE_OPTIONS+=(--ignore="${line}"); fi done <<< "${{ inputs.ignore }}" - ci-cd update-deps ${FAIL_FAST} \ + ci-cd update-deps ${FAIL_FAST} ${VERBOSE} \ --root-repo-path="${PWD}" \ + --ignore-separator="..." \ "${IGNORE_OPTIONS[@]}" + if [ "${{ inputs.debug }}" == "true" ]; then + git status + fi + if [ -n "$(git status --porcelain pyproject.toml)" ]; then echo "UPDATE_DEPS=true" >> $GITHUB_ENV git add pyproject.toml @@ -113,6 +141,10 @@ jobs: - name: Set PR body if: env.UPDATE_DEPS == 'true' run: | + if [ "${{ inputs.debug }}" == "true" ]; then + set -x + fi + if [ -z "${{ inputs.pr_body_file }}" ] || [ ! -f "${{ inputs.pr_body_file }}" ]; then PR_BODY_FILE=.tmp_pr-body_${{ github.run_id }}_${{ github.run_number }}_${{ github.run_attempt }}.txt echo "PR_BODY_FILE=${PR_BODY_FILE}" >> $GITHUB_ENV diff --git a/ci_cd/tasks/update_deps.py b/ci_cd/tasks/update_deps.py index 8cf5e173..eafa21c6 100644 --- a/ci_cd/tasks/update_deps.py +++ b/ci_cd/tasks/update_deps.py @@ -52,6 +52,7 @@ "Value to use instead of ellipsis (`...`) as a separator in `--ignore` " "key/value-pairs." ), + "verbose": "Whether or not to print debug statements.", }, iterable=["ignore"], ) @@ -62,6 +63,7 @@ def update_deps( # pylint: disable=too-many-branches,too-many-locals,too-many-s pre_commit=False, ignore=None, ignore_separator="...", + verbose=False, ): """Update dependencies in specified Python package's `pyproject.toml`.""" if TYPE_CHECKING: # pragma: no cover @@ -70,10 +72,16 @@ def update_deps( # pylint: disable=too-many-branches,too-many-locals,too-many-s fail_fast: bool = fail_fast # type: ignore[no-redef] pre_commit: bool = pre_commit # type: ignore[no-redef] ignore_separator: str = ignore_separator # type: ignore[no-redef] + verbose: bool = verbose # type: ignore[no-redef] if not ignore: ignore: list[str] = [] # type: ignore[no-redef] + if verbose: + LOGGER.setLevel(logging.DEBUG) + LOGGER.addHandler(logging.StreamHandler(sys.stdout)) + LOGGER.debug("Verbose logging enabled.") + VersionSpec = namedtuple( "VersionSpec", [ @@ -84,6 +92,7 @@ def update_deps( # pylint: disable=too-many-branches,too-many-locals,too-many-s "version", "extra_operator_version", "environment_marker", + "spacing", ], ) @@ -134,7 +143,8 @@ def update_deps( # pylint: disable=too-many-branches,too-many-locals,too-many-s error = False for line in dependencies: match = re.match( - r"^(?P(?P[a-zA-Z0-9_.-]+)(?:\s*\[.*\])?)\s*" + r"^(?P(?P[a-zA-Z0-9_.-]+)(?:\s*\[.*\])?)" + r"(?P\s*)" r"(?:" r"(?P@\s*\S+)|" r"(?P>|<|<=|>=|==|!=|~=)\s*" @@ -267,16 +277,27 @@ def update_deps( # pylint: disable=too-many-branches,too-many-locals,too-many-s escaped_full_dependency_name = version_spec.full_dependency.replace( "[", r"\[" ).replace("]", r"\]") - update_file( - pyproject_path, - ( - rf'"{escaped_full_dependency_name} {version_spec.operator}.*"', - f'"{version_spec.full_dependency} ' - f"{version_spec.operator}{updated_version}" - f'{version_spec.extra_operator_version if version_spec.extra_operator_version else ""}' # pylint: disable=line-too-long - f'{version_spec.environment_marker if version_spec.environment_marker else ""}"', # pylint: disable=line-too-long - ), + + LOGGER.debug("updated_version: %s", updated_version) + LOGGER.debug( + "escaped_full_dependency_name: %s", escaped_full_dependency_name + ) + + pattern_sub_line = ( + rf'"{escaped_full_dependency_name}{version_spec.spacing}' + rf'{version_spec.operator}.*"' ) + replacement_sub_line = ( + f'"{version_spec.full_dependency}{version_spec.spacing}' + f"{version_spec.operator}{updated_version}" + f'{version_spec.extra_operator_version if version_spec.extra_operator_version else ""}' # pylint: disable=line-too-long + f'{version_spec.environment_marker if version_spec.environment_marker else ""}"' # pylint: disable=line-too-long + ) + + LOGGER.debug("pattern_sub_line: %s", pattern_sub_line) + LOGGER.debug("replacement_sub_line: %s", replacement_sub_line) + + update_file(pyproject_path, (pattern_sub_line, replacement_sub_line)) already_handled_packages.add(version_spec.package) updated_packages[version_spec.full_dependency] = ( f"{version_spec.operator}{updated_version}" diff --git a/docs/hooks/update_pyproject.md b/docs/hooks/update_pyproject.md index 01be3d22..df5e2917 100644 --- a/docs/hooks/update_pyproject.md +++ b/docs/hooks/update_pyproject.md @@ -55,6 +55,7 @@ Any of these options can be given through the `args` key when defining the hook. | `--fail-fast` | Fail immediately if an error occurs. Otherwise, print and ignore all non-critical errors. | No | `False` | _boolean_ | | `--ignore` | Ignore-rules based on the `ignore` config option of Dependabot.

It should be of the format: `key=value...key=value`, i.e., an ellipsis (`...`) separator and then equal-sign-separated key/value-pairs.
Alternatively, the `--ignore-separator` can be set to something else to overwrite the ellipsis.

The only supported keys are: `dependency-name`, `versions`, and `update-types`.

Can be supplied multiple times per `dependency-name`. | No | | _string_ | | `--ignore-separator` | Value to use instead of ellipsis (`...`) as a separator in `--ignore` key/value-pairs. | No | | _string_ | +| `--verbose` | Whether or not to print debug statements. | No | `False` | _boolean_ | ## Usage example diff --git a/docs/workflows/ci_check_pyproject_dependencies.md b/docs/workflows/ci_check_pyproject_dependencies.md index a2f94013..7b024c6b 100644 --- a/docs/workflows/ci_check_pyproject_dependencies.md +++ b/docs/workflows/ci_check_pyproject_dependencies.md @@ -70,6 +70,7 @@ The repository contains the following: | `pr_labels` | A comma separated list of strings of GitHub labels to use for the created PR. | No | _Empty string_ | _string_ | | `ignore` | Create ignore conditions for certain dependencies. A multi-line string of ignore rules, where each line is an ellipsis-separated (`...`) string of key/value-pairs. One line per dependency. This option is similar to [the `ignore` option of Dependabot](https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#ignore).

See also [Single vs multi-line input](index.md#single-vs-multi-line-input). | No | _Empty string_ | _string_ | `branch_name_extension` | A string to append to the branch name of the created PR. Example: `'-my-branch'`. It will be appended after a forward slash, so the final branch name will be `ci/update-pyproject/-my-branch`. | No | _Empty string_ | _string_ | +| `debug` | Whether to run the workflow in debug mode, printing extra debug information. | No | `false` | _boolean_ | ## Secrets diff --git a/tests/tasks/test_update_deps.py b/tests/tasks/test_update_deps.py index faebf47f..fbcbf920 100644 --- a/tests/tasks/test_update_deps.py +++ b/tests/tasks/test_update_deps.py @@ -52,7 +52,7 @@ def test_update_deps(tmp_path: "Path", caplog: pytest.LogCaptureFixture) -> None ] dev = [ "mike >={original_dependencies['mike']},<3", - "pre-commit ~={original_dependencies['pre-commit']}", + "pre-commit~={original_dependencies['pre-commit']}", "pylint ~={original_dependencies['pylint']}", "test[testing]", ] @@ -65,7 +65,9 @@ def test_update_deps(tmp_path: "Path", caplog: pytest.LogCaptureFixture) -> None "name", "name1<=1", "name2>=3", - "name3>=3,<2", + # Multiple version specifiers are currently not supported. + # Follow issue #141 for updates. + # "name3>=3,<2", "name4@http://foo.com", "name5 [fred,bar] @ http://foo.com ; python_version=='2.7'", "name6[quux, strange];python_version<'2.7' and platform_version=='2'", @@ -117,11 +119,12 @@ def test_update_deps(tmp_path: "Path", caplog: pytest.LogCaptureFixture) -> None for line in dependencies: if any( - line.startswith(package_name) - for package_name in ["invoke ", "pytest ", "pre-commit "] + line.startswith(package_name) for package_name in ["invoke ", "pytest "] ): package_name = line.split(maxsplit=1)[0] assert line == f"{package_name} ~={original_dependencies[package_name]}" + elif "pre-commit" in line: + assert line == f"pre-commit~={original_dependencies['pre-commit']}" elif "tomlkit" in line: # Should be three version digits, since the original dependency had three. assert line == "tomlkit[test,docs] ~=1.0.0" @@ -149,9 +152,19 @@ def test_update_deps(tmp_path: "Path", caplog: pytest.LogCaptureFixture) -> None elif "name1" in line: assert line == "name1<=1" elif "name2" in line: - assert line == "name2>=3" + # name2 is altered by update_deps() to include the latest version (v1.2.3) + # even though it was originally specifying all versions above and including + # v3. + # Note, if a user wishes to avoid this alteration, the `--ignore` option can + # be used to specify the package (and version(s)) to ignore. + assert line == "name2>=1" elif "name3" in line: - assert line == "name3>=3,<2" + # Multiple version specifiers are currently not supported. + # Follow issue #141 for updates. + pytest.fail( + "name3 is commnted out in the test file and should not be present." + ) + # assert line == "name3>=3,<2" elif "name4" in line: assert line == "name4@http://foo.com" assert "'name4' is pinned to a URL and will be skipped" in caplog.text @@ -983,7 +996,7 @@ def test_ignore_version_fails() -> None: "mike": "mike >=1.0,<3", "pytest": "pytest ~=7.2", "pytest-cov": "pytest-cov ~=3.1", - "pre-commit": "pre-commit ~=2.20", + "pre-commit": "pre-commit~=2.20", "pylint": "pylint ~=2.14", "Sphinx": "Sphinx >=4.5.0,<6", }, @@ -996,7 +1009,7 @@ def test_ignore_version_fails() -> None: "mike": "mike >=1.0,<3", "pytest": "pytest ~=7.2", "pytest-cov": "pytest-cov ~=3.1", - "pre-commit": "pre-commit ~=2.20", + "pre-commit": "pre-commit~=2.20", "pylint": "pylint ~=2.14", "Sphinx": "Sphinx >=6.1.3,<6", }, @@ -1012,7 +1025,7 @@ def test_ignore_version_fails() -> None: "mike": "mike >=1.0,<3", "pytest": "pytest ~=7.2", "pytest-cov": "pytest-cov ~=3.1", - "pre-commit": "pre-commit ~=2.20", + "pre-commit": "pre-commit~=2.20", "pylint": "pylint ~=2.14", "Sphinx": "Sphinx >=6.1.3,<6", }, @@ -1025,7 +1038,7 @@ def test_ignore_version_fails() -> None: "mike": "mike >=1.0,<3", "pytest": "pytest ~=7.2", "pytest-cov": "pytest-cov ~=3.1", - "pre-commit": "pre-commit ~=2.20", + "pre-commit": "pre-commit~=2.20", "pylint": "pylint ~=2.13", "Sphinx": "Sphinx >=6.1.3,<6", }, @@ -1038,7 +1051,7 @@ def test_ignore_version_fails() -> None: "mike": "mike >=1.0,<3", "pytest": "pytest ~=7.1", "pytest-cov": "pytest-cov ~=3.1", - "pre-commit": "pre-commit ~=2.20", + "pre-commit": "pre-commit~=2.20", "pylint": "pylint ~=2.14", "Sphinx": "Sphinx >=6.1.3,<6", }, @@ -1051,7 +1064,7 @@ def test_ignore_version_fails() -> None: "mike": "mike >=1.0,<3", "pytest": "pytest ~=7.2", "pytest-cov": "pytest-cov ~=3.0", - "pre-commit": "pre-commit ~=2.20", + "pre-commit": "pre-commit~=2.20", "pylint": "pylint ~=2.14", "Sphinx": "Sphinx >=6.1.3,<6", # This should be fixed! }, @@ -1064,7 +1077,7 @@ def test_ignore_version_fails() -> None: "mike": "mike >=1.0,<3", "pytest": "pytest ~=7.2", "pytest-cov": "pytest-cov ~=3.1", - "pre-commit": "pre-commit ~=2.20", + "pre-commit": "pre-commit~=2.20", "pylint": "pylint ~=2.14", "Sphinx": "Sphinx >=4.5.0,<6", }, @@ -1127,7 +1140,7 @@ def test_ignore_rules_logic( "mike >={original_dependencies['mike']},<3", "pytest ~={original_dependencies['pytest']}", "pytest-cov ~={original_dependencies['pytest-cov']}", - "pre-commit ~={original_dependencies['pre-commit']}", + "pre-commit~={original_dependencies['pre-commit']}", "pylint ~={original_dependencies['pylint']}", "Sphinx >={original_dependencies['Sphinx']},<6", ] @@ -1165,8 +1178,59 @@ def test_ignore_rules_logic( for line in dependencies: for dependency, dependency_requirement in expected_result.items(): - if f"{dependency} " in line: + # Assert the dependency in the updated pyproject.toml file equals the + # expected value. + # We have to use a regular expression to match the dependency name as some + # dependency names are sub-strings of each other (like 'pytest' is a + # sub-string of 'pytest-cov'). + if re.match(rf"{re.escape(dependency)}\s*(~|>).*", line): assert line == dependency_requirement break else: pytest.fail(f"Unknown package in line: {line}") + + +@pytest.mark.parametrize("verbose_flag", [True, False]) +def test_verbose( + verbose_flag: bool, + tmp_path: Path, + capsys: pytest.CaptureFixture, + caplog: pytest.LogCaptureFixture, +) -> None: + """Check the verbose flag is respected. + + Any logged messaged should be written to stdout IF the verbose flag is set. + Check that any expected log messages are found both in the logs and in stdout. + + If the verbose flag is not set, the messages should ONLY appear in the logs. + """ + from invoke import MockContext + + from ci_cd.tasks import update_deps + + with pytest.raises( + SystemExit, + match=r".*Error: Could not find the Python package repository's 'pyproject.toml' file.*", + ): + update_deps(MockContext(), root_repo_path=str(tmp_path), verbose=verbose_flag) + + captured_output = capsys.readouterr() + + # Expected log messages - note the strings are sub-strings of the full log messages + assert ( + "Verbose logging enabled." in caplog.text + if verbose_flag + else "Verbose logging enabled." not in caplog.text + ) + assert "Parsed ignore rules:" in caplog.text + + # Assert the above messages are the only messages in the logs + assert len(caplog.messages) == 2 if verbose_flag else 1 + + # Go through the log messages and ensure they either are or are not in stdout + for log_message in caplog.messages: + assert ( + log_message in captured_output.out + if verbose_flag + else log_message not in captured_output.out + )