-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis 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
Tips
|
There was a problem hiding this 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
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
.yamllint
Outdated
There was a problem hiding this comment.
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.
@sourcery-ai review |
There was a problem hiding this 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
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 |
There was a problem hiding this comment.
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
os: | ||
default: ubuntu-latest | ||
description: The operating system to use | ||
required: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to remove os as a parameter here because this gets confusing in our case, note we are using custom poetry setup that I have written with ubuntu as base image. Is hasn't been tested on other OS liek OSX or windows. This input might cause errors due to incompatibility. Also I think in pythons case this just adds a level of complexity that should be abstracted from user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - we only support Linux, at least for the foreseeable future.
I think this applies to other actions as well.
file_path: | ||
description: File path for which the docs will be generated | ||
required: true |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to common, this way we will have only one way to spell check the repo.
Currently spell checking is linked as dep from pyproj but I think it would make more sense to add spellchecking as pre-commit check (pre commit are being used for non code checks) which will make alot of sense.
If we do plan on going this route, then maybe it would be a good ide to remove typos from cookicutter as we should not have multiple spellcheckers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All makes sense to me, including removing the spell checking from CC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should just be named integration as parent dir is already called code-test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All makes sense to me, including removing the spell checking from CC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
os: | ||
default: ubuntu-latest | ||
description: The operating system to use | ||
required: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - we only support Linux, at least for the foreseeable future.
I think this applies to other actions as well.
python_version: | ||
description: Python version to use | ||
default: "3.12" | ||
required: false | ||
poetry_install_options: | ||
description: Options for installing dependencies via poetry | ||
required: false | ||
default: "--only=code_quality --no-root" | ||
poetry_export_options: | ||
description: Options for exporting dependencies to check for hash | ||
changes for cache invalidation | ||
required: false | ||
default: "--only=code_quality" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way that we could get around having to define this in all actions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you mean poetry options, then no, but if you see the defaults are already set to what we use in cookiecutter, so if some one would want to use this action with cookicutter, they wont have to pass them either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well.... technically yes, but I am assuming people from outside the org might also use this action with CC and their own separate groups of deps, if we don't take that into consideration, yes we can definitely remove it.
@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. 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 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. |
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: