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

Fix Python Issues Flagged by pre-commit #82

Merged
merged 13 commits into from
Mar 19, 2024

Conversation

Matthew-Grayson
Copy link
Contributor

@Matthew-Grayson Matthew-Grayson commented Mar 15, 2024

🗣 Description

.github Changes

Integrate .env file into test_python job in backend.yml. This enables us to remove the public/private keys in mitmproxy_sign_requests.py and test_mitmproxy_sign_requests.py.
Note: this is a test RSA private key and not used in any deployed environment.

Other Changes

  • Remove private/public keys from mitmproxy_sign_request.py and mitmproxy_sign_request.py
  • Add error handling and timeout to get requests in cities.py and counties.py
  • Configure bandit to skip rule B101, "assert used", for test files
  • Configure flake8 to skip checking for docstrings in test files
  • Add docstrings to all other python modules and functions
  • Remove unused imports and variables
  • Organize/alphabetize libraries in requirements.txt files and imports.
  • Add types-requests to additional_dependencies for mypy.
  • Update Scrapy to address security vulnerability.

💭 Motivation and context

Closes issue #77
All python-related pre-commit hooks now pass.

🧪 Testing

Unit tests pass.
The following pre-commit checks pass:

  • bandit
  • black
  • flake8
  • isort
  • mypy
  • pyupgrade
  • requirements-txt-fixer

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

✅ Pre-merge checklist

  • Revert dependencies to default branches.
  • Finalize version.

✅ Post-merge checklist

  • Create a release.

@Matthew-Grayson Matthew-Grayson linked an issue Mar 15, 2024 that may be closed by this pull request
7 tasks
@Matthew-Grayson Matthew-Grayson changed the title Fix issues flagged by bandit and flake8. Fix pre-commit Issues Related to Python Mar 15, 2024
@Matthew-Grayson Matthew-Grayson self-assigned this Mar 15, 2024
@Matthew-Grayson Matthew-Grayson changed the title Fix pre-commit Issues Related to Python Fix Python Issues Flagged by pre-commit Mar 15, 2024
@Matthew-Grayson Matthew-Grayson marked this pull request as draft March 16, 2024 00:02
@Matthew-Grayson Matthew-Grayson marked this pull request as ready for review March 17, 2024 16:10
Copy link
Collaborator

@cduhn17 cduhn17 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

I noted a few things that should be taken care of before I can approve this PR, as well as a couple of things that don't apply to this PR specifically, but I felt I should mention.

.python-version Outdated Show resolved Hide resolved
.bandit.yml Outdated Show resolved Hide resolved
.flake8 Outdated Show resolved Hide resolved
backend/scripts/populateCountiesCities/main.py Outdated Show resolved Hide resolved
backend/worker/requirements.txt Show resolved Hide resolved
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of the items that I mentioned - looks good now! 👍

Approved with same caveat as #32:

@Matthew-Grayson stated that the remaining automated test failures will be addressed in future PRs.

@Matthew-Grayson Matthew-Grayson mentioned this pull request Mar 18, 2024
3 tasks
@Matthew-Grayson
Copy link
Contributor Author

@dav3r Of Course!

@schmelz21 schmelz21 merged commit 2a0c609 into develop Mar 19, 2024
13 of 18 checks passed
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.

Fix Python Issues Flagged by pre-commit
4 participants