Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add QA checks to workflow #14

Closed
2 tasks done
maxrake opened this issue Apr 4, 2022 · 0 comments · Fixed by #219
Closed
2 tasks done

Add QA checks to workflow #14

maxrake opened this issue Apr 4, 2022 · 0 comments · Fixed by #219
Assignees
Labels
medium priority This should be addressed soon task Task/chore unrelated to a bug or feature request

Comments

@maxrake
Copy link
Contributor

maxrake commented Apr 4, 2022

Description

Add Quality Assurance (QA) checks to a new or existing workflow. This should include formatting and linting checks for the main file types supported in this repository (e.g., Python, YAML, etc.).

Additional Details

  • Consider adding to the existing Test workflow before creating a new workflow
  • It may also be possible to create a distinct QA workflow that can be referenced by several of the other/existing workflows
  • Ensure output/reports are provided for the failing QA checks, such that it is possible to view the reason for the failure(s)
  • Consider adding a pre-commit config file for developer usage, to confirm adherence prior to committing or pushing code
  • The intent of this QA enforcement is to limit the amount of conversation in PRs related to formatting and linting concerns
    • The PR should have a failed check when QA is not met, preventing the ability to merge the PR
  • References/Inspiration:
  • Tools already added:
    • black
    • refurb
    • vulture
    • yamllint
    • ruff
      • bandit
      • flake8
      • isort
      • pylint
  • Tools to consider adding:
    • mypy (see Enforce type checking #238)
    • markdownlint
    • actionlint
    • hadolint
    • shellcheck
    • deptry
    • TypeScript and Deno formatters/linters (for the CLI extension code)
    • others (this list is non-exhaustive)

Acceptance Criteria

  • QA checks are enforced in CI
  • Documentation is updated
@maxrake maxrake added medium priority This should be addressed soon task Task/chore unrelated to a bug or feature request labels Apr 4, 2022
@maxrake maxrake self-assigned this Apr 4, 2022
maxrake added a commit that referenced this issue Aug 3, 2022
The `phylum-ci` script entry point already had most of the code for a `pre-commit` environment. This PR updated that code to account for some edge cases.

The `.pre-commit-hooks.yaml` file was added, with a single hook configuration defined for use in consuming repositories. This is a Python hook and works without any additional system-level dependencies. If the `phylum` CLI binary is installed locally, it will be used. Otherwise, the hook will install it.

There was an attempt to add a second hook which would make use of the `phylumio/phylum-ci` docker image. This proved too difficult to implement without overhauling the way the image is used in regards to entrypoints. The only real loss is for users who may not want to have the `phylum` CLI installed locally and prefer a self-contained Docker environment instead.

Up until now, the CI environments that have been implemented allow for output, in the form of review comments, to be posted as rendered markdown. The environments that don't use CI...`pre-commit` and `no-CI` so far...display their output in the terminal. Instead of writing separate output for these environments, a conversion utility library (`connect-markdown-renderer`) is used to render the existing markdown output in the terminal. Additionally, the labels for these environments were shortened to be more readable...in both the output as a link and the Phylum UI in the label dropdown menu.

A local git pre-commit hook configuration was added to this repository. This will help to dog-food the integration and understand the `pre-commit` environment more generally. Like all `pre-commit` configurations, this is opt-in for individual developers. More hooks may be added when #14 is tackled.

Other actions taken:

* Rename `poetry_update` workflow to `auto_updates`
* Update the `auto_updates` workflow
  * Enable auto updates of the pre-commit hooks to the latest tags
    * Use immutable hashes instead of tag names
  * Ensure commits by `phylum-bot` are signed
  * Rename the workflow and branch names
* Add git `pre-commit` documentation

Closes #35
maxrake added a commit that referenced this issue Apr 5, 2023
This change adds a new `QA` job to the existing `Test` workflow. It is
done in a way that the `Test rollup` job can continue to be an enforced
status check, but with the addition of this job. A new optional `tox`
test environment named `qa` was added and is called by the `QA` job. The
environment can also be called by users locally. It relies on simply
running `pre-commit` on the existing hooks defined in the repository:

* trim trailing whitespace
* fix end of files
* check yaml
* check for added large files
* `black`
* `pyupgrade`
* analyze lockfile with `phylum-ci`
  * This one is skipped

The `phylum-ci` pre-commit hook is skipped in the `QA` job since:

* The current GitHub integration expects to *only* run in a PR context
  * The `Test` workflow also includes `workflow_dispatch` and `push`
  * The integration will fail even if a `GITHUB_TOKEN` is provided
* The `phylum-ci` action will already be run for pull request triggers

It is possible this restriction can be lifted in the future if the
GitHub integration is updated to account for additional execution
contexts beyond pull requests.

The `pre-commit` package was added as a dependency in the `qa` group and
can therefore be kept under configuration control and used by `poetry`.
Documentation was updated to make all the new usage patterns explicit.

Additional QA checks and `pre-commit` hooks will be added separately.

Additional changes made include:

* Fix a missing version check normalization
  * When no CLI version is specified and no CLI is installed
  * The version was reported as `latest`
* Version is now current latest in "vMajor.Minor.Bugfix" format

Closes #14
maxrake added a commit that referenced this issue May 3, 2023
`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
maxrake added a commit that referenced this issue May 4, 2023
`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
of `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

Related to #14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium priority This should be addressed soon task Task/chore unrelated to a bug or feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant