From 080edec66741210abb69b5b1b2382102559bfc0f Mon Sep 17 00:00:00 2001 From: Charles Coggins Date: Wed, 3 May 2023 17:47:36 -0500 Subject: [PATCH] chore: add `ruff` to QA checks `ruff` is the hot new linter for Python. It is written in Rust and is extremely fast. It also has tremendous support and constant updates. It replaces some of the existing (`pyupgrade`) and planned (e.g., `bandit`, `flake8`, `isort`, and `pylint`) QA tools that are outlined in #14. More new rules are added all the time. The initial configuration enables `ALL` rules by default and then selectively ignores specific rules based on conflicts, preference, and existing issues in the backlog. Using `ALL` has the risk that new rules may be enabled when `ruff` is updated but updates are automated, with a PR that includes enforced QA checks, to weekly dependency and pre-commit hook bumps. Therefore, the real risk is that a developer on this project may have a local version `ruff` installed that is newer than the one used for the `pre-commit` hook. That should limit the exposure to no more than a week of "early" rules and may still result in improved code quality. The list of actions taken in this PR include: * Add a custom `ruff` configuration in `pyproject.toml` * All rules are enabled by default * Specific rules and rule sets are disabled * Remove the `pyupgrade` pre-commit hook * Those checks are covered by `ruff` * Add the `ruff` pre-commit hook * This will enforce the rules in the QA check * Enable GitHub PR annotations for any findings * Add `noqa` comments, with reasons, where appropriate * Sort imports based on the `isort` configuration * Adhere to PEP-257 docstrings everywhere * Update the `CONTRIBUTING` guide to discuss QA expectations * Make updates to resolve all outstanding findings * Add a `black` badge to the `README` to show the formatting style used --- .github/workflows/test.yml | 2 ++ .pre-commit-config.yaml | 7 ++--- CONTRIBUTING.md | 6 ++++ README.md | 2 ++ pyproject.toml | 47 +++++++++++++++++++++++++++++ src/phylum/ci/ci_azure.py | 40 +++++++++++++----------- src/phylum/ci/ci_base.py | 45 +++++++++++++-------------- src/phylum/ci/ci_bitbucket.py | 10 +++--- src/phylum/ci/ci_github.py | 16 +++++----- src/phylum/ci/ci_gitlab.py | 8 ++--- src/phylum/ci/ci_none.py | 8 ++--- src/phylum/ci/ci_precommit.py | 12 +++++--- src/phylum/ci/cli.py | 11 +++---- src/phylum/ci/common.py | 8 ++--- src/phylum/ci/git.py | 46 +++++++++++++++++----------- src/phylum/ci/lockfile.py | 33 ++++++++++++++------ src/phylum/constants.py | 2 +- src/phylum/github.py | 47 ++++++++++++++--------------- src/phylum/init/cli.py | 27 ++++++++--------- src/phylum/init/sig.py | 2 +- tests/__init__.py | 4 +++ tests/functional/__init__.py | 5 +++ tests/functional/test_ci.py | 13 ++++---- tests/functional/test_init.py | 15 +++++---- tests/unit/__init__.py | 4 +++ tests/unit/test_git.py | 4 +-- tests/unit/test_package_metadata.py | 6 ++-- tests/unit/test_sig.py | 33 +++++++++----------- 28 files changed, 280 insertions(+), 183 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 370272c5..7510e34d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -51,6 +51,8 @@ jobs: # * The current GitHub integration expects to *only* be run in a PR context # * The `phylum-ci` action will already be run for pull request triggers SKIP: phylum-ci + # Add annotations to the PR for any findings + RUFF_FORMAT: github run: poetry run tox run -e qa test-matrix: diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 2fe68f1b..c7ef0244 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -21,11 +21,10 @@ repos: hooks: - id: black - - repo: https://github.com/asottile/pyupgrade - rev: 7c551d39dfa69851607d85392ffa296568e92436 # frozen: v3.3.2 + - repo: https://github.com/charliermarsh/ruff-pre-commit + rev: aad2f97dee13b682f514df846844b374cdd06e7c # frozen: v0.0.264 hooks: - - id: pyupgrade - args: [--py38-plus] + - id: ruff - repo: https://github.com/dosisod/refurb rev: 7fb404137a6dbb8c7b346ffd904db5c17b1c24ed # frozen: v1.16.0 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8dbcaa5c..b12be863 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -213,6 +213,12 @@ The pull request should work for Python 3.8, 3.9, 3.10, and 3.11. Check and make sure that the tests pass for all supported Python versions. +To ensure quality assurance (QA), a series of checks are performed in a `qa` test. +This test essentially runs the `pre-commit` hooks to ensure proper formatting and linting over the repo. +Sometimes it is necessary to bypass these checks (e.g., via `# noqa` directives). These exceptions should +be rare and include a reason in the comment for the exclusion. Create a new issue and reference that, when +more detail is needed or the exclusion is meant to be temporary. + The [Semantic Pull Requests](https://github.com/apps/semantic-pull-requests) GitHub app is in use for this repository as a means to ensure each PR that gets merged back to the `main` branch adheres to the [conventional commit](https://www.conventionalcommits.org) strategy. This means that either the PR title (when diff --git a/README.md b/README.md index 0756a334..382f3945 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,7 @@ [![GitHub Workflow Status (branch)][workflow_shield]][workflow_test] [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)][CoC] [![pre-commit](https://img.shields.io/badge/pre--commit-enabled-brightgreen?logo=pre-commit)][pre-commit] +[![Code style: black](https://img.shields.io/badge/code%20style-black-000000.svg)][black] [![Downloads](https://pepy.tech/badge/phylum/month)][downloads] [![Discord](https://img.shields.io/discord/1070071012353376387?logo=discord)][discord_invite] @@ -19,6 +20,7 @@ Utilities for integrating Phylum into CI pipelines (and beyond) [workflow_test]: https://github.com/phylum-dev/phylum-ci/actions/workflows/test.yml [CoC]: https://github.com/phylum-dev/phylum-ci/blob/main/CODE_OF_CONDUCT.md [pre-commit]: https://github.com/pre-commit/pre-commit +[black]: https://github.com/psf/black [downloads]: https://pepy.tech/project/phylum [discord_invite]: https://discord.gg/Fe6pr5eW6p diff --git a/pyproject.toml b/pyproject.toml index af6993e7..9d9e1869 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -90,6 +90,53 @@ paths = ["src", "tests"] python_version = "3.8" format = "github" +[tool.ruff] +# Reference: https://beta.ruff.rs/docs/settings +line-length = 120 +target-version = "py38" +force-exclude = true +src = ["src", "tests"] +select = [ + # Using `ALL` has the risk that new rules may be enabled when `ruff` is updated but updates are + # automated, with a PR that includes enforced QA checks, to weekly dependency and pre-commit hook bumps. + "ALL" +] +ignore = [ + # Reference: https://beta.ruff.rs/docs/rules/ + # + # `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Prefer D211. + "D203", # one-blank-line-before-class + # `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Prefer D212. + "D213", # multi-line-summary-second-line + # Cached instance methods are okay in this project b/c instances are short lived and won't lead to memory leaks. + "B019", # cached-instance-method + # Assigning to a variable before a return statement is more readable and useful for debugging + "RET504", # unnecessary-assign + # These ignores will be removed during https://github.com/phylum-dev/phylum-ci/issues/238 + "ANN", # flake8-annotations + "TCH", # flake8-type-checking + # These ignores will be removed during https://github.com/phylum-dev/phylum-ci/issues/47 + "EM", # flake8-errmsg + "TRY", # tryceratops + # This ignore will be removed during https://github.com/phylum-dev/phylum-ci/issues/23 + "T20", # flake8-print +] + +[tool.ruff.per-file-ignores] +"test_*.py" = [ + # It is expected to use `assert` statements in `pytest` test code + "S101", # assert + # `subprocess` input is controlled in test code + "S603", # subprocess-without-shell-equals-true +] + +[tool.ruff.isort] +force-sort-within-sections = true + +[tool.ruff.pydocstyle] +# Use PEP-257 style docstrings +convention = "pep257" + [tool.semantic_release] # Reference: https://python-semantic-release.readthedocs.io/en/latest/configuration.html branch = "main" diff --git a/src/phylum/ci/ci_azure.py b/src/phylum/ci/ci_azure.py index be4cc73e..308c356c 100644 --- a/src/phylum/ci/ci_azure.py +++ b/src/phylum/ci/ci_azure.py @@ -15,15 +15,15 @@ * https://learn.microsoft.com/rest/api/azure/devops/git/pull-request-threads/list * https://learn.microsoft.com/rest/api/azure/devops/git/pull-request-threads/create """ +from argparse import Namespace import base64 +from functools import cached_property, lru_cache import os import re import shlex import subprocess +from typing import Optional, Tuple import urllib.parse -from argparse import Namespace -from functools import cached_property, lru_cache -from typing import Optional import requests @@ -84,7 +84,7 @@ def is_in_pr() -> bool: class CIAzure(CIBase): """Provide methods for an Azure Pipelines CI environment.""" - def __init__(self, args: Namespace) -> None: + def __init__(self, args: Namespace) -> None: # noqa: D107 ; the base __init__ docstring is better here super().__init__(args) self.ci_platform_name = "Azure Pipelines" if is_in_pr(): @@ -164,17 +164,7 @@ def common_ancestor_commit(self) -> Optional[str]: remote = git_remote() if is_in_pr(): - # There is no single predefined variable available to provide the PR base SHA. - # Instead, it can be determined with a `git merge-base` command, like is done for the CINone implementation. - # Reference: https://learn.microsoft.com/azure/devops/pipelines/build/variables - src_branch = os.getenv("SYSTEM_PULLREQUEST_SOURCEBRANCH", "") - tgt_branch = os.getenv("SYSTEM_PULLREQUEST_TARGETBRANCH", "") - if not src_branch: - raise SystemExit(" [!] The SYSTEM_PULLREQUEST_SOURCEBRANCH environment variable must exist and be set") - if not tgt_branch: - raise SystemExit(" [!] The SYSTEM_PULLREQUEST_TARGETBRANCH environment variable must exist and be set") - print(f" [+] SYSTEM_PULLREQUEST_SOURCEBRANCH: {src_branch}") - print(f" [+] SYSTEM_PULLREQUEST_TARGETBRANCH: {tgt_branch}") + src_branch, tgt_branch = get_pr_branches() else: # Assume the working context is within a CI triggered build environment when not in a PR. @@ -221,7 +211,7 @@ def common_ancestor_commit(self) -> Optional[str]: cmd = ["git", "merge-base", src_branch, tgt_branch] print(f" [*] Finding common ancestor commit with command: {shlex.join(cmd)}") try: - common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() + common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() # noqa: S603 except subprocess.CalledProcessError as err: ref_url = "https://learn.microsoft.com/azure/devops/pipelines/yaml-schema/steps-checkout#shallow-fetch" print(f" [!] The common ancestor commit could not be found: {err}") @@ -282,6 +272,22 @@ def post_output(self) -> None: post_github_comment(comments_url, self.github_token, self.analysis_report) +def get_pr_branches() -> Tuple[str, str]: + """Get the source and destination branches when in a PR context and return them as a tuple.""" + # There is no single predefined variable available to provide the PR base SHA. + # Instead, it can be determined with a `git merge-base` command, like is done for the CINone implementation. + # Reference: https://learn.microsoft.com/azure/devops/pipelines/build/variables + src_branch = os.getenv("SYSTEM_PULLREQUEST_SOURCEBRANCH", "") + tgt_branch = os.getenv("SYSTEM_PULLREQUEST_TARGETBRANCH", "") + if not src_branch: + raise SystemExit(" [!] The SYSTEM_PULLREQUEST_SOURCEBRANCH environment variable must exist and be set") + if not tgt_branch: + raise SystemExit(" [!] The SYSTEM_PULLREQUEST_TARGETBRANCH environment variable must exist and be set") + print(f" [+] SYSTEM_PULLREQUEST_SOURCEBRANCH: {src_branch}") + print(f" [+] SYSTEM_PULLREQUEST_TARGETBRANCH: {tgt_branch}") + return src_branch, tgt_branch + + def post_azure_comment(azure_token: str, comment: str) -> None: """Post a comment on an Azure Repos Pull Request (PR). @@ -366,7 +372,7 @@ def post_azure_comment(azure_token: str, comment: str) -> None: "parentCommentId": 0, "content": comment, "commentType": "text", - } + }, ], "status": "active", } diff --git a/src/phylum/ci/ci_base.py b/src/phylum/ci/ci_base.py index 17ed90ed..a67746de 100644 --- a/src/phylum/ci/ci_base.py +++ b/src/phylum/ci/ci_base.py @@ -4,22 +4,22 @@ integration (CI) environment. Common functionality is provided where possible and CI specific features are designated as abstract methods to be defined in specific CI environments. """ +from abc import ABC, abstractmethod +from argparse import Namespace +from collections import OrderedDict +from functools import cached_property import json import os +from pathlib import Path import shlex import shutil import subprocess import tempfile import textwrap -from abc import ABC, abstractmethod -from argparse import Namespace -from collections import OrderedDict -from functools import cached_property -from pathlib import Path from typing import List, Optional -import pathspec from packaging.version import Version +import pathspec from rich.markdown import Markdown from ruamel.yaml import YAML @@ -131,7 +131,7 @@ def filter_lockfiles(self, provided_lockfiles: List[Path]) -> Lockfiles: cmd = [str(self.cli_path), "parse", str(provided_lockfile)] # stdout and stderr are piped to DEVNULL because we only care about the return code. # pylint: disable-next=subprocess-run-check ; we want return code here and don't want to raise when non-zero - if bool(subprocess.run(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL).returncode): + if bool(subprocess.run(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL).returncode): # noqa: S603 print(f" [!] Provided lockfile failed to parse as a known lockfile type: {provided_lockfile}") continue lockfiles.append(Lockfile(provided_lockfile, self.cli_path, self.common_ancestor_commit)) @@ -235,7 +235,7 @@ def cli_path(self) -> Path: # Ensure stdout is piped to DEVNULL, to keep the token from being printed in (CI log) output. cmd = [str(cli_path), "auth", "token"] # pylint: disable-next=subprocess-run-check ; we want the return code here and don't want to raise when non-zero - if bool(subprocess.run(cmd, stdout=subprocess.DEVNULL).returncode): + if bool(subprocess.run(cmd, stdout=subprocess.DEVNULL).returncode): # noqa: S603 raise SystemExit(" [!] A Phylum API key is required to continue.") print(f" [+] Using Phylum CLI instance: {cli_version} at {cli_path}") @@ -256,7 +256,7 @@ def phylum_label(self) -> str: * Start the label with the `self.ci_platform_name` * Replace all runs of whitespace characters with a single `-` character """ - raise NotImplementedError() + raise NotImplementedError @property def lockfile_hash_object(self) -> str: @@ -285,7 +285,7 @@ def common_ancestor_commit(self) -> Optional[str]: When found, it should be returned as a string of the SHA1 sum representing the commit. When it can't be found (or there is an error), `None` should be returned. """ - raise NotImplementedError() + raise NotImplementedError @property @abstractmethod @@ -294,7 +294,7 @@ def is_any_lockfile_changed(self) -> bool: Implementations should return `True` if any lockfile has changed and `False` otherwise. """ - raise NotImplementedError() + raise NotImplementedError def update_lockfiles_change_status(self, commit: str, err_msg: Optional[str] = None) -> None: """Update each lockfile's change status. @@ -308,7 +308,7 @@ def update_lockfiles_change_status(self, commit: str, err_msg: Optional[str] = N # `--exit-code` will make git exit with 1 if there were differences while 0 means no differences. # Any other exit code is an error and a reason to re-raise. cmd = ["git", "diff", "--exit-code", "--quiet", commit, "--", str(lockfile.path)] - ret = subprocess.run(cmd, check=False) + ret = subprocess.run(cmd, check=False) # noqa: S603 if ret.returncode == 0: print(f" [-] The lockfile `{lockfile!r}` has not changed") lockfile.is_lockfile_changed = False @@ -349,15 +349,16 @@ def ensure_project_exists(self) -> None: if self.phylum_group: print(f" [-] Using Phylum group: {self.phylum_group}") cmd = [str(self.cli_path), "project", "create", "--group", self.phylum_group, self.phylum_project] - ret = subprocess.run(cmd, check=False, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + ret = subprocess.run(cmd, check=False, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) # noqa: S603 # The Phylum CLI will return a unique error code of 14 when a project that already # exists is attempted to be created. This situation is recognized and allowed to happen # since it means the project exists as expected. Any other exit code is an error. + cli_exit_code_project_already_exists = 14 if ret.returncode == 0: print(f" [-] Project {self.phylum_project} created successfully.") if self._project_file_already_existed: print(f" [!] Overwrote previous `.phylum_project` file found at: {self._phylum_project_file}") - elif ret.returncode == 14: + elif ret.returncode == cli_exit_code_project_already_exists: print(f" [-] Project {self.phylum_project} already exists. Continuing with it ...") else: print(f" [!] There was a problem creating the project with command: {shlex.join(cmd)}") @@ -407,11 +408,12 @@ def analyze(self) -> ReturnCode: print(f" [*] Performing analysis with command: {shlex.join(cmd)}") try: - analysis_result = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout + analysis_result = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout # noqa: S603 except subprocess.CalledProcessError as err: # The Phylum project will fail analysis if the configured policy criteria are not met. # This causes the return code to be 100 and lands us here. Check for this case to proceed. - if err.returncode == 100: + cli_exit_code_failed_policy = 100 + if err.returncode == cli_exit_code_failed_policy: analysis_result = err.stdout else: print(f" [!] stdout:\n{err.stdout}") @@ -430,13 +432,12 @@ def analyze(self) -> ReturnCode: else: print(" [+] The analysis is complete and there were NO failures") returncode = ReturnCode.SUCCESS + elif analysis.is_failure: + print(" [!] There were failures in one or more completed packages") + returncode = ReturnCode.FAILURE_INCOMPLETE else: - if analysis.is_failure: - print(" [!] There were failures in one or more completed packages") - returncode = ReturnCode.FAILURE_INCOMPLETE - else: - print(" [+] There were no failures in the packages that have completed so far") - returncode = ReturnCode.INCOMPLETE + print(" [+] There were no failures in the packages that have completed so far") + returncode = ReturnCode.INCOMPLETE return returncode diff --git a/src/phylum/ci/ci_bitbucket.py b/src/phylum/ci/ci_bitbucket.py index fc319321..545cea50 100644 --- a/src/phylum/ci/ci_bitbucket.py +++ b/src/phylum/ci/ci_bitbucket.py @@ -16,14 +16,14 @@ * https://developer.atlassian.com/cloud/bitbucket/rest/api-group-pullrequests/ * https://developer.atlassian.com/cloud/bitbucket/rest/intro/#pullrequest """ +from argparse import Namespace +from functools import cached_property, lru_cache import os import re import shlex import subprocess -import urllib.parse -from argparse import Namespace -from functools import cached_property, lru_cache from typing import Optional +import urllib.parse import requests @@ -61,7 +61,7 @@ def is_in_pr() -> bool: class CIBitbucket(CIBase): """Provide methods for a Bitbucket Pipelines environment.""" - def __init__(self, args: Namespace) -> None: + def __init__(self, args: Namespace) -> None: # noqa: D107 ; the base __init__ docstring is better here super().__init__(args) self.ci_platform_name = "Bitbucket Pipelines" if is_in_pr(): @@ -167,7 +167,7 @@ def common_ancestor_commit(self) -> Optional[str]: cmd = ["git", "merge-base", src_branch, tgt_branch] print(f" [*] Finding common ancestor commit with command: {shlex.join(cmd)}") try: - common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() + common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() # noqa: S603 except subprocess.CalledProcessError as err: ref_url = "https://support.atlassian.com/bitbucket-cloud/docs/git-clone-behavior/" print(f" [!] The common ancestor commit could not be found: {err}") diff --git a/src/phylum/ci/ci_github.py b/src/phylum/ci/ci_github.py index bcb01cdd..84c9fd95 100644 --- a/src/phylum/ci/ci_github.py +++ b/src/phylum/ci/ci_github.py @@ -10,12 +10,13 @@ * https://docs.github.com/en/rest/pulls/comments * https://docs.github.com/en/rest/guides/working-with-comments#pull-request-comments """ +from argparse import Namespace +from functools import cached_property import json import os +from pathlib import Path import re import subprocess -from argparse import Namespace -from functools import cached_property from typing import Optional import requests @@ -42,13 +43,13 @@ class CIGitHub(CIBase): """Provide methods for a GitHub Actions environment.""" - def __init__(self, args: Namespace) -> None: + def __init__(self, args: Namespace) -> None: # noqa: D107 ; the base __init__ docstring is better here # This is the recommended workaround for container actions, to avoid the `unsafe repository` error. # It is added before super().__init__(args) so that lockfile change detection will be set properly. # See https://github.com/actions/checkout/issues/766 (git CVE-2022-24765) for more detail. github_workspace = os.getenv("GITHUB_WORKSPACE", "/github/workspace") cmd = ["git", "config", "--global", "--add", "safe.directory", github_workspace] - subprocess.run(cmd, check=True) + subprocess.run(cmd, check=True) # noqa: S603 super().__init__(args) self.ci_platform_name = "GitHub Actions" @@ -76,10 +77,11 @@ def _check_prerequisites(self) -> None: # https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#pull_request if os.getenv("GITHUB_EVENT_NAME") != "pull_request": raise SystemExit(" [!] The workflow event must be `pull_request`") - github_event_path = os.getenv("GITHUB_EVENT_PATH") - if github_event_path is None: + github_event_path_envvar = os.getenv("GITHUB_EVENT_PATH") + if github_event_path_envvar is None: raise SystemExit(" [!] Could not read the `GITHUB_EVENT_PATH` environment variable") - with open(github_event_path, encoding="utf-8") as f: + github_event_path = Path(github_event_path_envvar) + with github_event_path.open(encoding="utf-8") as f: pr_event = json.load(f) self._pr_event = pr_event diff --git a/src/phylum/ci/ci_gitlab.py b/src/phylum/ci/ci_gitlab.py index 6c006428..20bf9397 100644 --- a/src/phylum/ci/ci_gitlab.py +++ b/src/phylum/ci/ci_gitlab.py @@ -7,12 +7,12 @@ * https://docs.gitlab.com/ee/ci/jobs/ci_job_token.html * https://docs.gitlab.com/ee/api/notes.html#merge-requests """ +from argparse import Namespace +from functools import cached_property, lru_cache import os import re import shlex import subprocess -from argparse import Namespace -from functools import cached_property, lru_cache from typing import Optional import requests @@ -44,7 +44,7 @@ def is_in_mr() -> bool: class CIGitLab(CIBase): """Provide methods for a GitLab CI environment.""" - def __init__(self, args: Namespace) -> None: + def __init__(self, args: Namespace) -> None: # noqa: D107 ; the base __init__ docstring is better here super().__init__(args) self.ci_platform_name = "GitLab CI" if is_in_mr(): @@ -128,7 +128,7 @@ def common_ancestor_commit(self) -> Optional[str]: cmd = ["git", "merge-base", src_branch, default_branch] print(f" [*] Finding common ancestor commit with command: {shlex.join(cmd)}") try: - common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() + common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() # noqa: S603 except subprocess.CalledProcessError as err: ref_url = "https://docs.gitlab.com/ee/ci/runners/configure_runners.html#git-strategy" print(f" [!] The common ancestor commit could not be found: {err}") diff --git a/src/phylum/ci/ci_none.py b/src/phylum/ci/ci_none.py index b1db71ac..856e769a 100644 --- a/src/phylum/ci/ci_none.py +++ b/src/phylum/ci/ci_none.py @@ -5,10 +5,10 @@ This is also the fallback implementation to use when no known CI platform is detected. """ import argparse -import re -import subprocess from functools import cached_property from pathlib import Path +import re +import subprocess from typing import Optional from phylum.ci.ci_base import CIBase @@ -18,7 +18,7 @@ class CINone(CIBase): """Provide methods for operating outside of a known CI environment.""" - def __init__(self, args: argparse.Namespace) -> None: + def __init__(self, args: argparse.Namespace) -> None: # noqa: D107 ; the base __init__ docstring is better here super().__init__(args) self.ci_platform_name = "No CI" @@ -50,7 +50,7 @@ def common_ancestor_commit(self) -> Optional[str]: remote = git_remote() cmd = ["git", "merge-base", "HEAD", f"refs/remotes/{remote}/HEAD"] try: - common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() + common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() # noqa: S603 except subprocess.CalledProcessError as err: print(f" [!] The common ancestor commit could not be found: {err}") print(f" [!] stdout:\n{err.stdout}") diff --git a/src/phylum/ci/ci_precommit.py b/src/phylum/ci/ci_precommit.py index 6ab4f0c8..8fb0a097 100644 --- a/src/phylum/ci/ci_precommit.py +++ b/src/phylum/ci/ci_precommit.py @@ -9,11 +9,11 @@ * https://pre-commit.com/index.html#arguments-pattern-in-hooks """ import argparse +from functools import cached_property +from pathlib import Path import re import subprocess import sys -from functools import cached_property -from pathlib import Path from typing import List, Optional from phylum.ci.ci_base import CIBase @@ -23,7 +23,8 @@ class CIPreCommit(CIBase): """Provide methods for operating within a pre-commit hook.""" - def __init__(self, args: argparse.Namespace, remainder: List[str]) -> None: + def __init__(self, args: argparse.Namespace, remainder: List[str]) -> None: # noqa: D107 + # The base __init__ docstring is better here self.extra_args = remainder super().__init__(args) self.ci_platform_name = "pre-commit" @@ -37,7 +38,8 @@ def _check_prerequisites(self) -> None: super()._check_prerequisites() cmd = ["git", "diff", "--cached", "--name-only"] - staged_files = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.strip().split("\n") + output = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout # noqa: S603 + staged_files = output.strip().split("\n") extra_arg_paths = [Path(extra_arg).resolve() for extra_arg in self.extra_args] print(" [*] Checking extra args for valid pre-commit scenarios ...") @@ -87,7 +89,7 @@ def common_ancestor_commit(self) -> Optional[str]: """Find the common ancestor commit.""" cmd = ["git", "rev-parse", "--verify", "HEAD"] try: - common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() + common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() # noqa: S603 except subprocess.CalledProcessError as err: print(f" [!] The common ancestor commit could not be found: {err}") print(f" [!] stdout:\n{err.stdout}") diff --git a/src/phylum/ci/cli.py b/src/phylum/ci/cli.py index 65ba4666..e0891e66 100644 --- a/src/phylum/ci/cli.py +++ b/src/phylum/ci/cli.py @@ -162,7 +162,7 @@ def get_args(args: Optional[Sequence[str]] = None) -> Tuple[argparse.Namespace, def main(args: Optional[Sequence[str]] = None) -> int: - """Main entrypoint.""" + """Provide the main entrypoint.""" parsed_args, remainder_args = get_args(args=args) # Perform version check and normalization here so as to minimize GitHub API calls when @@ -182,12 +182,11 @@ def main(args: Optional[Sequence[str]] = None) -> int: print(f" [+] lockfile(s) in use: {ci_env.lockfiles}") if ci_env.force_analysis: print(" [+] Forced analysis specified with flag or otherwise set. Proceeding with analysis ...") + elif ci_env.is_any_lockfile_changed: + print(" [+] A lockfile has changed. Proceeding with analysis ...") else: - if ci_env.is_any_lockfile_changed: - print(" [+] A lockfile has changed. Proceeding with analysis ...") - else: - print(" [+] No lockfile has changed. Nothing to do.") - return 0 + print(" [+] No lockfile has changed. Nothing to do.") + return 0 # Generate a label to use for analysis and report it print(f" [+] Label to use for analysis: {ci_env.phylum_label}") diff --git a/src/phylum/ci/common.py b/src/phylum/ci/common.py index 104d07e7..ec7ca5c7 100644 --- a/src/phylum/ci/common.py +++ b/src/phylum/ci/common.py @@ -1,7 +1,7 @@ """Provide common data structures for the package.""" import dataclasses -import json from enum import IntEnum +import json from typing import List @@ -11,7 +11,7 @@ class PackageDescriptor: name: str version: str - type: str + type: str # noqa: A003 ; shadowing built-in `type` is okay since renaming here would be more confusing # Type alias @@ -24,7 +24,7 @@ class JobPolicyEvalResult: is_failure: bool incomplete_count: int - output: str # noqa: F841 ; unused attribute, but kept to maintain consistency with the corresponding Phylum type + output: str report: str @@ -41,7 +41,7 @@ class ReturnCode(IntEnum): class DataclassJSONEncoder(json.JSONEncoder): """Custom JSON Encoder class that is able to serialize dataclass objects.""" - def default(self, o): + def default(self, o): # noqa: D102 ; the parent's docstring is better here if dataclasses.is_dataclass(o): return dataclasses.asdict(o) return super().default(o) diff --git a/src/phylum/ci/git.py b/src/phylum/ci/git.py index 4f462664..24d269ea 100644 --- a/src/phylum/ci/git.py +++ b/src/phylum/ci/git.py @@ -1,8 +1,7 @@ """Provide common git functions.""" -import os -import subprocess from pathlib import Path +import subprocess from typing import List, Optional @@ -26,8 +25,8 @@ def git_remote(git_c_path: Optional[Path] = None) -> str: This function is limited in that it will only work when there is a single remote defined. A RuntimeError exception will be raised when there is not exactly one remote. """ - cmd = git_base_cmd(git_c_path) + ["remote"] - remotes = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.splitlines() + cmd = [*git_base_cmd(git_c_path), "remote"] + remotes = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.splitlines() # noqa: S603 if not remotes: raise RuntimeError("No git remotes configured") if len(remotes) > 1: @@ -47,9 +46,9 @@ def git_set_remote_head(remote: str, git_c_path: Optional[Path] = None) -> None: """ base_cmd = git_base_cmd(git_c_path=git_c_path) print(" [*] Automatically setting the remote HEAD ref ...") - cmd = base_cmd + ["remote", "set-head", remote, "--auto"] + cmd = [*base_cmd, "remote", "set-head", remote, "--auto"] try: - subprocess.run(cmd, check=True, capture_output=True) + subprocess.run(cmd, check=True, capture_output=True) # noqa: S603 except subprocess.CalledProcessError as err: print(f" [!] Setting the remote HEAD failed: {err}") print(f" [!] stdout:\n{err.stdout}") @@ -68,15 +67,25 @@ def git_default_branch_name(remote: str, git_c_path: Optional[Path] = None) -> s """ base_cmd = git_base_cmd(git_c_path=git_c_path) prefix = f"refs/remotes/{remote}/" - cmd = base_cmd + ["symbolic-ref", f"{prefix}HEAD"] + cmd = [*base_cmd, "symbolic-ref", f"{prefix}HEAD"] try: - default_branch_name = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.strip() + default_branch_name = subprocess.run( + cmd, # noqa: S603 + check=True, + text=True, + capture_output=True, + ).stdout.strip() except subprocess.CalledProcessError: # The most likely problem is that the remote HEAD ref is not set. The attempt to set it here, inside # the except block, is due to wanting to minimize calling commands that require git credentials. print(" [!] Failed to get the remote HEAD ref. It is likely not set. Attempting to set it and try again ...") git_set_remote_head(remote) - default_branch_name = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.strip() + default_branch_name = subprocess.run( + cmd, # noqa: S603 + check=True, + text=True, + capture_output=True, + ).stdout.strip() # Starting with Python 3.9, the str.removeprefix() method was introduced to do this same thing if default_branch_name.startswith(prefix): @@ -94,8 +103,8 @@ def git_curent_branch_name(git_c_path: Optional[Path] = None) -> str: path instead of the current working directory, which is the default when not provided. """ base_cmd = git_base_cmd(git_c_path=git_c_path) - cmd = base_cmd + ["branch", "--show-current"] - current_branch = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.strip() + cmd = [*base_cmd, "branch", "--show-current"] + current_branch = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.strip() # noqa: S603 return current_branch @@ -107,8 +116,8 @@ def git_hash_object(object_path: Path, git_c_path: Optional[Path] = None) -> str """ base_cmd = git_base_cmd(git_c_path=git_c_path) # Reference: https://git-scm.com/book/en/v2/Git-Internals-Git-Objects - cmd = base_cmd + ["hash-object", str(object_path)] - hash_object = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.strip() + cmd = [*base_cmd, "hash-object", str(object_path)] + hash_object = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.strip() # noqa: S603 return hash_object @@ -133,16 +142,17 @@ def git_repo_name(git_c_path: Optional[Path] = None) -> str: try: remote = git_remote(git_c_path=git_c_path) is_remote_defined = True - cmd = base_cmd + ["remote", "get-url", remote] + cmd = [*base_cmd, "remote", "get-url", remote] except RuntimeError as err: print(f" [!] {err}. Will get the repo name from the local repository instead.") is_remote_defined = False - cmd = base_cmd + ["rev-parse", "--show-toplevel"] + cmd = [*base_cmd, "rev-parse", "--show-toplevel"] - full_repo_name = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.strip() + full_repo_name = subprocess.run(cmd, check=True, text=True, capture_output=True).stdout.strip() # noqa: S603 - repo_name = os.path.basename(full_repo_name) + full_repo_path = Path(full_repo_name) + repo_name = full_repo_path.name if is_remote_defined and repo_name.endswith(".git"): - repo_name, _ = os.path.splitext(repo_name) + repo_name = full_repo_path.stem return repo_name diff --git a/src/phylum/ci/lockfile.py b/src/phylum/ci/lockfile.py index accdb9e8..88a537e4 100644 --- a/src/phylum/ci/lockfile.py +++ b/src/phylum/ci/lockfile.py @@ -3,13 +3,13 @@ A Phylum project can consist of multiple lockfiles. This class represents a single lockfile. """ +from functools import cached_property, lru_cache import json +from pathlib import Path import shutil import subprocess import tempfile import textwrap -from functools import cached_property, lru_cache -from pathlib import Path from typing import List, Optional, TypeVar from phylum.ci.common import PackageDescriptor, Packages @@ -36,14 +36,14 @@ def __repr__(self) -> str: """Return a debug printable string representation of the `Lockfile` object.""" # NOTE: Any change from this format should be made carefully as caller's # may be relying on `repr(lockfile)` to provide the relative path. - # Example: print(f"Relative path to lockfile: `{lockfile!r}`") + # Example: print(f"Relative path to lockfile: `{lockfile!r}`") # noqa: ERA001 ; commented code intended return str(self.path.relative_to(Path.cwd())) def __str__(self) -> str: """Return the nicely printable string representation of the `Lockfile` object.""" # NOTE: Any change from this format should be made carefully as caller's # may be relying on `str(lockfile)` to provide the path. - # Example: print(f"Path to lockfile: `{lockfile}`") + # Example: print(f"Path to lockfile: `{lockfile}`") # noqa: ERA001 ; commented code intended return str(self.path) def __lt__(self: Self, other: Self) -> bool: @@ -120,7 +120,7 @@ def current_lockfile_packages(self) -> Packages: """Get the current lockfile packages.""" try: cmd = [str(self.cli_path), "parse", str(self.path)] - parse_result = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() + parse_result = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() # noqa: S603 except subprocess.CalledProcessError as err: print(f" [!] There was an error running the command: {' '.join(err.cmd)}") print(f" [!] stdout:\n{err.stdout}") @@ -141,7 +141,12 @@ def previous_lockfile_object(self) -> Optional[str]: try: # Use the `repr` form to get the relative path to the lockfile cmd = ["git", "rev-parse", "--verify", f"{self.common_ancestor_commit}:{self!r}"] - prev_lockfile_object = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() + prev_lockfile_object = subprocess.run( + cmd, # noqa: S603 + check=True, + capture_output=True, + text=True, + ).stdout.strip() except subprocess.CalledProcessError as err: # There could be a true error, but the working assumption when here is a previous version does not exist print(f" [?] There *may* be an issue with the attempt to get the previous lockfile object: {err}") @@ -157,7 +162,12 @@ def get_previous_lockfile_packages(self, prev_lockfile_object: str) -> Packages: with tempfile.NamedTemporaryFile(mode="w+") as prev_lockfile_fd: try: cmd = ["git", "cat-file", "blob", prev_lockfile_object] - prev_lockfile_contents = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout + prev_lockfile_contents = subprocess.run( + cmd, # noqa: S603 + check=True, + capture_output=True, + text=True, + ).stdout prev_lockfile_fd.write(prev_lockfile_contents) prev_lockfile_fd.flush() except subprocess.CalledProcessError as err: @@ -168,7 +178,12 @@ def get_previous_lockfile_packages(self, prev_lockfile_object: str) -> Packages: return [] try: cmd = [str(self.cli_path), "parse", prev_lockfile_fd.name] - parse_result = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip() + parse_result = subprocess.run( + cmd, # noqa: S603 + check=True, + capture_output=True, + text=True, + ).stdout.strip() except subprocess.CalledProcessError as err: print(f" [!] There was an error running the command: {' '.join(err.cmd)}") print(f" [!] stdout:\n{err.stdout}") @@ -178,7 +193,7 @@ def get_previous_lockfile_packages(self, prev_lockfile_object: str) -> Packages: [!] Due to error, assuming no previous lockfile packages. Please report this as a bug if you believe `{self!r}` is a valid lockfile at revision `{self.common_ancestor_commit}`. - """ + """ # noqa: COM812 ; FP due to multiline string in function call ) print(msg) return [] diff --git a/src/phylum/constants.py b/src/phylum/constants.py index 667d8700..d2fae619 100644 --- a/src/phylum/constants.py +++ b/src/phylum/constants.py @@ -65,7 +65,7 @@ # Environment variable name to hold the Phylum CLI token used to access the backend API. # The API token can also be set via the environment variable `PHYLUM_API_KEY`, which will take precedence over # the `offline_access` parameter in the `settings.yaml` file. -ENVVAR_NAME_TOKEN = "PHYLUM_API_KEY" # nosec ; this is NOT a hard-coded password +ENVVAR_NAME_TOKEN = "PHYLUM_API_KEY" # noqa: S105 ; this is NOT a hard-coded password # Environment variable name to hold the URI of the Phylum API instance used to access the backend API. ENVVAR_NAME_API_URI = "PHYLUM_API_URI" diff --git a/src/phylum/github.py b/src/phylum/github.py index 7b118955..ae5fea17 100644 --- a/src/phylum/github.py +++ b/src/phylum/github.py @@ -75,29 +75,28 @@ def github_request( rate_limit_reset_header = int(time.mktime(time.localtime()) + seconds_in_hour) reset_time = time.asctime(time.localtime(rate_limit_reset_header)) - if resp.status_code == requests.codes.forbidden: - # There are several reasons why a 403 status code (FORBIDDEN) could be returned: - # * API rate limit exceeded - # * API secondary rate limit exceeded - # * Failed login limit exceeded - # * Requests with no `User-Agent` header - # Reference: https://docs.github.com/rest/overview/resources-in-the-rest-api - - # The most likely reason is that the rate limit has been exceeded so check for that. - # The other possible forbidden cases are not common enough to check for here. - # Reference: https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting - if rate_limit_remaining == "0": - msg = textwrap.dedent( - f"""\ - [!] GitHub API rate limit of {rate_limit} requests/hour was exceeded for URL: {api_url} - The current time is: {current_time} - Rate limit resets at: {reset_time} - Options include waiting to try again after the rate limit resets or to make authenticated - requests by providing a GitHub token in the `GITHUB_TOKEN` environment variable. Reference: - {PAT_REF} - """ - ) - raise SystemExit(msg) + # There are several reasons why a 403 status code (FORBIDDEN) could be returned: + # * API rate limit exceeded + # * API secondary rate limit exceeded + # * Failed login limit exceeded + # * Requests with no `User-Agent` header + # Reference: https://docs.github.com/rest/overview/resources-in-the-rest-api + # + # The most likely reason is that the rate limit has been exceeded so check for that. + # The other possible forbidden cases are not common enough to check for here. + # Reference: https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting + if resp.status_code == requests.codes.forbidden and rate_limit_remaining == "0": + msg = textwrap.dedent( + f"""\ + [!] GitHub API rate limit of {rate_limit} requests/hour was exceeded for URL: {api_url} + The current time is: {current_time} + Rate limit resets at: {reset_time} + Options include waiting to try again after the rate limit resets or to make authenticated + requests by providing a GitHub token in the `GITHUB_TOKEN` environment variable. Reference: + {PAT_REF} + """ # noqa: COM812 ; FP due to multiline string in function call + ) + raise SystemExit(msg) # TODO: Make this a debug level log output statement # https://github.com/phylum-dev/phylum-ci/issues/23 @@ -112,7 +111,7 @@ def github_request( [!] A bad request was made to the GitHub API: {err} Response text: {resp.text.strip()} - """ + """ # noqa: COM812 ; FP due to multiline string in function call ) raise SystemExit(msg) from err diff --git a/src/phylum/init/cli.py b/src/phylum/init/cli.py index c1bcdfce..0519ef09 100644 --- a/src/phylum/init/cli.py +++ b/src/phylum/init/cli.py @@ -1,20 +1,20 @@ """Console script for phylum-init.""" import argparse +from functools import lru_cache import os import pathlib +from pathlib import Path import platform import shutil import subprocess import sys import tempfile -import zipfile -from functools import lru_cache -from pathlib import Path from typing import List, Optional, Tuple +import zipfile -import requests from packaging.utils import canonicalize_version from packaging.version import InvalidVersion, Version +import requests from ruamel.yaml import YAML from phylum import __version__ @@ -58,7 +58,7 @@ def get_expected_phylum_bin_path(): def get_phylum_cli_version(cli_path: Path) -> str: """Get the version of the installed and active Phylum CLI and return it.""" cmd = [str(cli_path), "--version"] - version = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip().lower() + version = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip().lower() # noqa: S603 # Starting with Python 3.9, the str.removeprefix() method was introduced to do this same thing prefix = "phylum " @@ -236,9 +236,8 @@ def is_token_set(phylum_settings_path, token=None): if configured_token is None: return False - if token is not None: - if token != configured_token: - return False + if token is not None and token != configured_token: + return False return True @@ -274,7 +273,7 @@ def process_token_option(args): def setup_token(token: str) -> None: - """Setup the CLI credentials with a provided token.""" + """Configure the CLI credentials with a provided token.""" phylum_settings_path = get_phylum_settings_path() ensure_settings_file() yaml = YAML() @@ -297,7 +296,7 @@ def ensure_settings_file() -> None: if phylum_bin_path is None: raise SystemExit(" [!] Could not find the path to the Phylum CLI. Unable to ensure the settings file.") cmd = [str(phylum_bin_path), "version"] - subprocess.run(cmd, check=True) + subprocess.run(cmd, check=True) # noqa: S603 def process_uri_option(args: argparse.Namespace) -> None: @@ -340,13 +339,13 @@ def confirm_setup() -> None: if is_token_set(get_phylum_settings_path()): # Check that the token and API URI were setup correctly by using them to display the current auth status cmd = [str(phylum_bin_path), "auth", "status"] - subprocess.run(cmd, check=True) + subprocess.run(cmd, check=True) # noqa: S603 else: print(" [!] Existing token not found. Can't confirm setup.") # Print the help message to aid log review cmd = [str(phylum_bin_path), "--help"] - subprocess.run(cmd, check=True) + subprocess.run(cmd, check=True) # noqa: S603 def get_args(args=None): @@ -413,7 +412,7 @@ def get_args(args=None): def main(args=None): - """Main entrypoint.""" + """Provide the main entrypoint.""" args = get_args(args=args) # Perform version check and normalization here so as to minimize GitHub API calls when @@ -475,7 +474,7 @@ def main(args=None): cmd = ["install", "-m", "0755", "phylum", "/usr/local/bin/phylum"] else: cmd = ["sh", "install.sh"] - subprocess.run(cmd, check=True, cwd=extracted_dir) + subprocess.run(cmd, check=True, cwd=extracted_dir) # noqa: S603 process_uri_option(args) process_token_option(args) diff --git a/src/phylum/init/sig.py b/src/phylum/init/sig.py index 39856d39..af59a000 100644 --- a/src/phylum/init/sig.py +++ b/src/phylum/init/sig.py @@ -44,7 +44,7 @@ CPPnPlCwuCIyUPezCP5XYczuHfaWeuwArlwdJFSUpMTc+SqO6REKgL9yvpqsO5Ia sQIDAQAB -----END PUBLIC KEY----- - """ + """ # noqa: COM812 ; FP due to multiline string in function call ), encoding="ASCII", ) diff --git a/tests/__init__.py b/tests/__init__.py index e69de29b..25222771 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -0,0 +1,4 @@ +"""Create a package for tests. + +This package is not intended to be included when building a wheel, but it will be when building a source distribution. +""" diff --git a/tests/functional/__init__.py b/tests/functional/__init__.py index e69de29b..326ab4a8 100644 --- a/tests/functional/__init__.py +++ b/tests/functional/__init__.py @@ -0,0 +1,5 @@ +"""Create a package for functional tests. + +The tests in this package are meant to confirm the functionality of individual `phylum` packages. They can be bigger and +slower than unit tests. It is more acceptable to use `subprocess` calls here to confirm behavior. +""" diff --git a/tests/functional/test_ci.py b/tests/functional/test_ci.py index 5ba7fdac..efd5b519 100644 --- a/tests/functional/test_ci.py +++ b/tests/functional/test_ci.py @@ -4,8 +4,7 @@ from phylum import __version__ from phylum.ci import SCRIPT_NAME - -from ..constants import PYPROJECT +from tests.constants import PYPROJECT def test_run_as_module(): @@ -14,8 +13,8 @@ def test_run_as_module(): This is the `python -m ` format to "run library module as a script." NOTE: The is specified as the dotted path to the package where the `__main__.py` module exists. """ - cmd = f"{sys.executable} -m phylum.ci --help".split() - ret = subprocess.run(cmd) + cmd = [sys.executable, "-m", "phylum.ci", "--help"] + ret = subprocess.run(cmd, check=False) assert ret.returncode == 0, "Running the package as a module failed" @@ -24,7 +23,7 @@ def test_run_as_script(): scripts = PYPROJECT.get("tool", {}).get("poetry", {}).get("scripts", {}) assert scripts, "There should be at least one script entry point" assert SCRIPT_NAME in scripts, f"The {SCRIPT_NAME} script should be a defined entry point" - ret = subprocess.run([SCRIPT_NAME, "-h"]) + ret = subprocess.run([SCRIPT_NAME, "-h"], check=False) assert ret.returncode == 0, f"{SCRIPT_NAME} entry point failed" @@ -32,8 +31,8 @@ def test_version_option(): """Ensure the correct program name and version is displayed when using the `--version` option.""" # The argparse module adds a newline to the output expected_output = f"{SCRIPT_NAME} {__version__}\n" - cmd = f"{sys.executable} -m phylum.ci --version".split() - ret = subprocess.run(cmd, capture_output=True, encoding="utf-8") + cmd = [sys.executable, "-m", "phylum.ci", "--version"] + ret = subprocess.run(cmd, check=False, capture_output=True, encoding="utf-8") assert not ret.stderr, "Nothing should be written to stderr" assert ret.returncode == 0, "A non-successful return code was provided" assert ret.stdout == expected_output, "Output did not match expected input" diff --git a/tests/functional/test_init.py b/tests/functional/test_init.py index 88437f36..5b3fbc47 100644 --- a/tests/functional/test_init.py +++ b/tests/functional/test_init.py @@ -1,13 +1,12 @@ """"Test the phylum-init command line interface (CLI).""" +from pathlib import Path import subprocess import sys -from pathlib import Path from phylum import __version__ from phylum.init import SCRIPT_NAME, sig from phylum.init.cli import save_file_from_url - -from ..constants import PYPROJECT +from tests.constants import PYPROJECT def test_run_as_module(): @@ -16,8 +15,8 @@ def test_run_as_module(): This is the `python -m ` format to "run library module as a script." NOTE: The is specified as the dotted path to the package where the `__main__.py` module exists. """ - cmd = f"{sys.executable} -m phylum.init --help".split() - ret = subprocess.run(cmd) + cmd = [sys.executable, "-m", "phylum.init", "--help"] + ret = subprocess.run(cmd, check=False) assert ret.returncode == 0, "Running the package as a module failed" @@ -26,7 +25,7 @@ def test_run_as_script(): scripts = PYPROJECT.get("tool", {}).get("poetry", {}).get("scripts", {}) assert scripts, "There should be at least one script entry point" assert SCRIPT_NAME in scripts, f"The {SCRIPT_NAME} script should be a defined entry point" - ret = subprocess.run([SCRIPT_NAME, "-h"]) + ret = subprocess.run([SCRIPT_NAME, "-h"], check=False) assert ret.returncode == 0, f"{SCRIPT_NAME} entry point failed" @@ -34,8 +33,8 @@ def test_version_option(): """Ensure the correct program name and version is displayed when using the `--version` option.""" # The argparse module adds a newline to the output expected_output = f"{SCRIPT_NAME} {__version__}\n" - cmd = f"{sys.executable} -m phylum.init --version".split() - ret = subprocess.run(cmd, capture_output=True, encoding="utf-8") + cmd = [sys.executable, "-m", "phylum.init", "--version"] + ret = subprocess.run(cmd, check=False, capture_output=True, encoding="utf-8") assert not ret.stderr, "Nothing should be written to stderr" assert ret.returncode == 0, "A non-successful return code was provided" assert ret.stdout == expected_output, "Output did not match expected input" diff --git a/tests/unit/__init__.py b/tests/unit/__init__.py index e69de29b..08b481cb 100644 --- a/tests/unit/__init__.py +++ b/tests/unit/__init__.py @@ -0,0 +1,4 @@ +"""Create a package for unit tests. + +The tests in this package should be small, fast, and focused on testing individual modules or functions therein. +""" diff --git a/tests/unit/test_git.py b/tests/unit/test_git.py index ed575be7..cf90433e 100644 --- a/tests/unit/test_git.py +++ b/tests/unit/test_git.py @@ -2,8 +2,8 @@ from pathlib import Path -import pytest from dulwich import porcelain +import pytest from phylum.ci.git import git_repo_name @@ -16,7 +16,7 @@ # Names of a git repository that will be initialized locally # Separate lists are used because cloned local repos with a name ending in `.git` are not supported -INITIALIZED_LOCAL_REPO_NAMES = CLONED_LOCAL_REPO_NAMES + ["local_repo.git"] +INITIALIZED_LOCAL_REPO_NAMES = [*CLONED_LOCAL_REPO_NAMES, "local_repo.git"] @pytest.mark.parametrize("repo_name", INITIALIZED_LOCAL_REPO_NAMES) diff --git a/tests/unit/test_package_metadata.py b/tests/unit/test_package_metadata.py index 48828b76..30328221 100644 --- a/tests/unit/test_package_metadata.py +++ b/tests/unit/test_package_metadata.py @@ -6,8 +6,7 @@ from packaging.version import Version from phylum import PKG_NAME, PKG_SUMMARY, __author__, __email__, __version__ - -from ..constants import PYPROJECT +from tests.constants import PYPROJECT def test_project_version(): @@ -21,7 +20,8 @@ def test_python_version(): """Ensure the python version used to test is a supported version.""" supported_minor_versions = (8, 9, 10, 11) python_version = sys.version_info - assert python_version.major == 3, "Only Python 3 is supported" + acceptable_python_major_version = 3 + assert python_version.major == acceptable_python_major_version, "Only Python 3 is supported" assert python_version.minor in supported_minor_versions, "Attempting to run unsupported Python version" diff --git a/tests/unit/test_sig.py b/tests/unit/test_sig.py index 15a74c2d..4059446a 100644 --- a/tests/unit/test_sig.py +++ b/tests/unit/test_sig.py @@ -14,27 +14,24 @@ def test_phylum_pubkey_is_bytes(): def test_phylum_pubkey_is_constant(): """Ensure the RSA public key in use by Phylum has not changed.""" - expected_key = bytes( - dedent( - """\ - -----BEGIN PUBLIC KEY----- - MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAyGgvuy6CWSgJuhKY8oVz - 42udH1F2yIlaBoxAdQFuY2zxPSSpK9zv34B7m0JekuC5WCYfW0gS2Z8Ryu2RVdQh - 7DXvQb7qwzZT0H11K9Pw8hIHBvZPM+d61GWgWDc3k/rFwMmqd+kytVZy0mVxNdv4 - P2qvy6BNaiUI7yoB1ahR/6klfkPit0X7pkK9sTHwW+/WcYitTQKnEnRzA3q8EmA7 - rbU/sFEypzBA3C3qNJZyKSwy47kWXhC4xXUS2NXvew4FoVU6ybMoeDApwsx1AgTu - CPPnPlCwuCIyUPezCP5XYczuHfaWeuwArlwdJFSUpMTc+SqO6REKgL9yvpqsO5Ia - sQIDAQAB - -----END PUBLIC KEY----- - """ - ), - encoding="ASCII", - ) - assert sig.PHYLUM_RSA_PUBKEY == expected_key, "The key should not be changing" + key_text = """\ + -----BEGIN PUBLIC KEY----- + MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAyGgvuy6CWSgJuhKY8oVz + 42udH1F2yIlaBoxAdQFuY2zxPSSpK9zv34B7m0JekuC5WCYfW0gS2Z8Ryu2RVdQh + 7DXvQb7qwzZT0H11K9Pw8hIHBvZPM+d61GWgWDc3k/rFwMmqd+kytVZy0mVxNdv4 + P2qvy6BNaiUI7yoB1ahR/6klfkPit0X7pkK9sTHwW+/WcYitTQKnEnRzA3q8EmA7 + rbU/sFEypzBA3C3qNJZyKSwy47kWXhC4xXUS2NXvew4FoVU6ybMoeDApwsx1AgTu + CPPnPlCwuCIyUPezCP5XYczuHfaWeuwArlwdJFSUpMTc+SqO6REKgL9yvpqsO5Ia + sQIDAQAB + -----END PUBLIC KEY----- + """ + expected_key = bytes(dedent(key_text), encoding="ASCII") + assert expected_key == sig.PHYLUM_RSA_PUBKEY, "The key should not be changing" def test_phylum_pubkey_is_rsa(): """Ensure the public key in use by Phylum is in fact a 2048 bit RSA key.""" key = load_pem_public_key(sig.PHYLUM_RSA_PUBKEY) assert isinstance(key, rsa.RSAPublicKey) - assert key.key_size == 2048 + expected_rsa_pubkey_size_in_bits = 2048 + assert key.key_size == expected_rsa_pubkey_size_in_bits