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

feat: add Python CI #2

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

feat: add Python CI #2

wants to merge 13 commits into from

Conversation

JaeAeich
Copy link
Collaborator

@JaeAeich JaeAeich commented Sep 1, 2024

Add python ci

Summary by Sourcery

Introduce multiple CI workflows for Python projects, including setup, testing, code quality checks, vulnerability assessments, and package publishing to PyPI. These workflows utilize Poetry for dependency management and caching, and integrate tools like Bandit, mypy, ruff, and typos for various checks.

CI:

  • Add a CI workflow for setting up Python and Poetry, including dependency management and caching.
  • Introduce a documentation check workflow to ensure API documentation is up to date.
  • Implement an integration test workflow that sets up the environment, runs tests, and uploads coverage reports.
  • Add a unit test workflow to run unit tests and upload coverage reports.
  • Create a code vulnerability test workflow using Bandit to check for code vulnerabilities.
  • Introduce a type check workflow to ensure type safety and correctness using mypy.
  • Add a workflow to publish Python packages to PyPI upon release.
  • Implement a dependency vulnerability test workflow using Safety to check for vulnerabilities in dependencies.
  • Add a code formatting check workflow to ensure code adheres to style guidelines using ruff.
  • Introduce a linting workflow to ensure code quality and adherence to standards using ruff.
  • Add a spell check workflow to identify spelling errors in the codebase using typos.

Copy link

sourcery-ai bot commented Sep 1, 2024

Reviewer's Guide by Sourcery

This pull request introduces a comprehensive set of Python CI workflows and actions, focusing on setting up the environment, running various code quality checks, tests, and publishing to PyPI. The changes are implemented by adding multiple YAML configuration files for different GitHub Actions, utilizing the Poetry package manager for Python dependency management.

File-Level Changes

Change Details Files
Implement Poetry setup action for consistent Python environment configuration
  • Configure system, Python, and Poetry versions
  • Set up caching for pipx and Poetry dependencies
  • Install Poetry and project dependencies
  • Generate hash for required dependencies to optimize caching
actions/python/setup/poetry/action.yaml
Add documentation check action to verify API documentation is up to date
  • Generate API docs using sphinx-apidoc
  • Compare current docs with latest generated docs
  • Fail CI if documentation is out of date
actions/python/docs/autogenerated-docs-check/action.yaml
Implement integration and unit test actions with coverage reporting
  • Set up test environment using Poetry
  • Run tests using pytest with coverage
  • Upload coverage reports to Codecov
actions/python/code-test/integration-test/action.yaml
actions/python/code-test/unit-test/action.yaml
Add code and dependency vulnerability check actions
  • Implement Bandit for code vulnerability scanning
  • Use Safety for checking dependency vulnerabilities
actions/python/vulnerability/code-vulnerability/action.yaml
actions/python/vulnerability/dependency-vulnerability/action.yaml
Implement code quality check actions
  • Add type checking using mypy
  • Implement code formatting check using Ruff
  • Add linting action using Ruff
  • Implement spell checking using Typos
actions/python/code-quality/type-check/action.yaml
actions/python/code-quality/format/action.yaml
actions/python/code-quality/lint/action.yaml
actions/python/code-quality/spell-check/action.yaml
Add PyPI publishing action for releasing Python packages
  • Set up environment for package publishing
  • Implement Poetry-based package building and publishing to PyPI
actions/python/release/pypi/action.yaml

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JaeAeich - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider pinning the external action to a specific version or commit hash instead of using @main to ensure stability and avoid unexpected changes.
  • Enhance the pull request description to provide more context and details about the changes and their purpose.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

.yamllint Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: remove before merge and add this as another PR.

@JaeAeich
Copy link
Collaborator Author

JaeAeich commented Sep 1, 2024

@sourcery-ai review

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JaeAeich - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟡 Security: 1 issue found
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

- name: Compare current docs with latest docs
shell: bash
run: |
shasum /tmp/docs/source/pages/* > /tmp/docs.sha
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Using shasum for doc comparison may be overly sensitive

Consider using a more robust method for comparing documentation that isn't as sensitive to minor formatting changes.

        diff -rq /tmp/docs/source/pages/ ${{ inputs.auto_doc_dir }}/ | grep -v "Only in" > /tmp/docs_diff.txt
        if [ -s /tmp/docs_diff.txt ]; then
          echo "Differences found in documentation:"
          cat /tmp/docs_diff.txt
        else
          echo "No significant differences found in documentation."
        fi

actions/python/release/pypi/action.yaml Outdated Show resolved Hide resolved
Comment on lines 22 to 24
file_path:
description: File path for which the docs will be generated
required: true
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Make this an optional fields too, grep and awk name of the project from pyproject and search if that dir exists, if so take that as file_path otherwise check if user has provided a file_path, else err.

We should aim to make all the fields non required, or atleast the input should not depend on project, ie codecov_token will be secret.CODECOV_TOKEN regardless of the project hence this will be easier to template. Essentially if we can ideally make all the deafults such that their is no values passed that are related to project, we can template all the CI work in one single repo.

@JaeAeich
Copy link
Collaborator Author

JaeAeich commented Sep 1, 2024

TODO: Remember to add below code in example ci, below code stops previously running ci if a new commit is pushed, saving ci time.

concurrency:
  group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
  cancel-in-progress: true

@JaeAeich
Copy link
Collaborator Author

JaeAeich commented Sep 7, 2024

@uniqueg Im testing this CI on my private repo, and another good feature that I think would be nice to implement would be to add debug steps for CI. So basically for each CI that fails, comment on the PR, what went wrong or, how to improve it, or who to tag and ask for help. Check out this example.

image

I propose, for each CI we document the most common way to fix it. Lets say the format CI fails, the bot will comment, please run make fl, if you are using windows install poetry, and run this command etc etc, the debug steps. This will be on Doccumantion hub obv, and the PR link it.

Then we also let the PR creator know who to tag, this we fetch from a list of mainteiners from somewhere (a gitst perhaphs), this way we can have updated list of maintainer for a particular language or porject, maybe, lets say I have a gist

uniqueg

I provide gist link to the actions, and it fetches, does data sanitisation, and suggest the PR creator to tag the mainter if in need.

Thoughts??

PS: Yess Im testing on windows as well, imma make it compatible even if we don;t use it.

@uniqueg uniqueg changed the title feat: add python ci feat: add Python ci Oct 11, 2024
@uniqueg uniqueg changed the title feat: add Python ci feat: add Python CI Oct 11, 2024
@uniqueg
Copy link
Member

uniqueg commented Oct 11, 2024

@uniqueg Im testing this CI on my private repo, and another good feature that I think would be nice to implement would be to add debug steps for CI. So basically for each CI that fails, comment on the PR, what went wrong or, how to improve it, or who to tag and ask for help. Check out this example.

image

I propose, for each CI we document the most common way to fix it. Lets say the format CI fails, the bot will comment, please run make fl, if you are using windows install poetry, and run this command etc etc, the debug steps. This will be on Doccumantion hub obv, and the PR link it.

Then we also let the PR creator know who to tag, this we fetch from a list of mainteiners from somewhere (a gitst perhaphs), this way we can have updated list of maintainer for a particular language or porject, maybe, lets say I have a gist

uniqueg

I provide gist link to the actions, and it fetches, does data sanitisation, and suggest the PR creator to tag the mainter if in need.

Thoughts??

PS: Yess Im testing on windows as well, imma make it compatible even if we don;t use it.

Hi @JaeAeich - this sounds all great.

However, let's keep incremental improvements. Long PRs always take long to get over the line. Let's see that we merge this one soon - I don't think it depends on this idea.

@JaeAeich
Copy link
Collaborator Author

@uniqueg, yeah makes sense, I've some more improvements but I'll have a safe point here. The commenting func is done, some other things mentioned aren't. Please go through the PR, I have some more changes but those probably won't require reviews. I would prefer to merge this pr first so I can use python does using poetry action in common and rust ci.

Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already looked at these and they look good to me, so I'm okay with merging. It's a bit hard to review these and spot mistakes are sth, so I guess we'll have to see how they perform in the wild - and if they're issues, we can address them.

@JaeAeich
Copy link
Collaborator Author

@uniqueg There are a couple of changes that need to be made, pretty easy, but would be good feat and need discussion so mentioning them. The current poetry action that we have assumes that it is running on a proj with poetry stack or atleast pyporject.toml, but I wanna extend it to also take in only list of deps, why? so that we are able to use its caching feat for different env where we need python deps, eg rust project, or py project that doesn;t have pyproject.
Im not sure how that to structure its API though, currently we have:

      - name: Set up environment
        uses: ./.github/actions/setup/poetry
        with:
          os: ${{ job.os }}
          python-version: '3.11'
          poetry-install-options: "--with=test" # Defines the group to install, using poetry's api, eg with mean install main as 
          # well as test.
          poetry-export-options: "--with=test" # Defines which group to look at for cache invalidation via poetry's api, why 
          # a separate arg, one of the reason is because sometimes we don;t care if the main deps have changes as the 
          # other group mainly focuses on if or not cache should be invalidated. 

Ill probably add an extra arg callled deps, but since I haven't tested this I would wait before merge, and also I saw some minor issues in logs while testing this so I need to get that too.

// thoughts?

For the time being if oyu can, please go through the comments for each action, is there a specific way you'd like the commetns to be? Those are all obv scrath and rough commetns. If you can give me one good comment (basically set the standards) for any action, I think I can make the rest.

@JaeAeich
Copy link
Collaborator Author

I have made changes to poetry setupr and also tested it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants