Fix violations of static analysis for pytests#29
Open
jakub-vavra-cz wants to merge 1 commit into
Open
Conversation
Reviewer's GuideAdds a static code analysis job to the main CI workflow, configures Ruff and mypy in pyproject, and adjusts pytest sudo/security tests (including sudo alias usage and assertion formatting) to satisfy Ruff/isort/typing checks while removing the old standalone static-code-analysis workflow. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
3b27935 to
487c167
Compare
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the
static-code-analysisjob, the venv creation and dependency installation logic closely mirrors what you already do in thepytestjob; consider refactoring this into a shared reusable step or composite action to avoid duplication and future drift. - The workflow installs
ruffandmypywithout version pins, which can cause CI to break when new releases introduce changes; consider pinning these tools inrequirements.txtor a separate constraints file for reproducible analysis runs. - The
sudoersstring intest_cve__mailer_escalationis now a single f-string with embedded newlines, which is harder to read and edit; consider switching to a multi-line literal (e.g., withtextwrap.dedent) to keep each directive on its own line in the source.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `static-code-analysis` job, the venv creation and dependency installation logic closely mirrors what you already do in the `pytest` job; consider refactoring this into a shared reusable step or composite action to avoid duplication and future drift.
- The workflow installs `ruff` and `mypy` without version pins, which can cause CI to break when new releases introduce changes; consider pinning these tools in `requirements.txt` or a separate constraints file for reproducible analysis runs.
- The `sudoers` string in `test_cve__mailer_escalation` is now a single f-string with embedded newlines, which is harder to read and edit; consider switching to a multi-line literal (e.g., with `textwrap.dedent`) to keep each directive on its own line in the source.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Enable static analysis on pushes and pull requests. Fix some isort and typing issues. Switch static analysis to ruff.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enable static analysis on pushes and pull requests. Fix some isort and typing issues.
Summary by Sourcery
Enable static analysis for the pytest test suite and align test and configuration code with the new linting and typing requirements.
Enhancements:
Build:
CI: