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

Maintenance week updates #239

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Maintenance week updates #239

merged 4 commits into from
Jun 6, 2024

Conversation

jonavellecuerdo
Copy link
Contributor

@jonavellecuerdo jonavellecuerdo commented Jun 4, 2024

What does this PR do?

Update app according to DataEng Application Maintenance. Here are some changes worth noting:

  • Update application to Python 3.12
    • All unit tests, including integration tests are passing 🎉
  • Address minimal linting errors
    • Update syntax in pyproject.toml
    • Address ruff rule violation for E501 in conftest.py
  • Clean up Makefile
  • Update README (Development and Environment Variables section)

How can a reviewer manually see the effects of these changes?

  1. Run make test and verify all unit tests are passing.
  2. Run integration tests and verify all integration tests are passing.

Includes new or updated dependencies?

YES

Developer

  • All new ENV is documented in README
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

* .pre-commit-config.yaml
* Dockerfile
* .python-version
* pyproject.toml
@coveralls
Copy link

coveralls commented Jun 4, 2024

Pull Request Test Coverage Report for Build 9375162046

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 8103188080: 0.0%
Covered Lines: 140
Relevant Lines: 140

💛 - Coveralls

@jonavellecuerdo jonavellecuerdo marked this pull request as ready for review June 5, 2024 14:03
Copy link
Contributor

@ghukill ghukill left a comment

Choose a reason for hiding this comment

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

I got nothing, looks good! Able to run tests, lenting, and integration tests locally.

Feel additionally confident given that the python 3.12 actually used is an image public.ecr.aws/lambda/python:3.12 created by AWS, for Lambdas, so expect it to work well.

Running the above with no env variables passed should result in an exception.
Running the above with no env variables passed should result in an exception.

## Environment Variables
Copy link
Contributor

Choose a reason for hiding this comment

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

Two thoughts:

1- Appreciate the centralization and moving of these here

2- While it's a lot of text... I still think I like it. As a developer, for example, I have the option of multiple-cursors and can quickly clear the comments. Or, if I'm InfraEng (and curious what they actually think) I can see all the env vars quickly as a list for confirming they are set as needed in terraform.

@coveralls
Copy link

coveralls commented Jun 6, 2024

Pull Request Test Coverage Report for Build 9401857322

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 8103188080: 0.0%
Covered Lines: 140
Relevant Lines: 140

💛 - Coveralls

@jonavellecuerdo jonavellecuerdo merged commit 87ac27d into main Jun 6, 2024
3 checks passed
@jonavellecuerdo jonavellecuerdo deleted the maintenance-week-updates branch June 6, 2024 13:36
@jonavellecuerdo
Copy link
Contributor Author

jonavellecuerdo commented Jun 7, 2024

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.

None yet

4 participants