-
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
In 1065 maintenance 09 2024 #272
Conversation
bb00b6a
to
4d01464
Compare
Pull Request Test Coverage Report for Build 10946501804Details
💛 - Coveralls |
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.
Changes look good to me! Thanks for the detailed PR, easy to follow along the changes.
I'm happy to approve -- tests and linting passes locally -- but @jonavellecuerdo it might be worth tagging Adam S. as a reviewer on this PR as well. He's likely in a better condition to perform any manual tests (if possible).
Adam S. doesn't do anything with this one, just SAP invoices. I still respond to all of the tickets related to this app |
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.
Great extra enhancements beyond the usual dependency and Python updates!
* Update Python version to 3.12 * Update Python dependencies * Update pyproject.toml and add Ruff rule G004 to 'ignore' list * Clean up section headers in Makefile * Update 'Environment Variables' and description in README
Why these changes are being introduced: * Use a central Config class for dynamic access to environment variables and simplify method for configuring loggers. How this addresses that need: * Create a Config class for dynamically accessing environment variables * Deprecate load_alma_config and update AlmaClient to use Config class to dynamically set attributes (base_url, headers, timeout) * Update configure_logger to use 'verbose' boolean flag * Update CLI to accept '-v/--verbose' boolean option and remove '-l/--log-level' string option * Add/update corresponding unit tests Side effects of this change: * Remove LOG_LEVEL as an optional environment variable Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1065
4d01464
to
bb3bd47
Compare
Purpose and background context
Update app according to DataEng Application Maintenance.
The major updates for this work include:
Config
class to support dynamic access to environment variablesAlmaClient
to useConfig
class (and deprecatingconfig.load_alma_config
Note: I opted for minimal updates and didn't convert the class to
attrs
hence the use of@property
methods for the class attributes that relied on access to env variables viaConfig
.configure_logger
to useverbose
boolean flag-l/--log-level
CLI option and updating CLI to use-v/--verbose
CLI option instead.Note: The ECS task definition in Prod currently sets
LOG_LEVEL
. This will not cause an issue asConfig().LOG_LEVEL
is not called anywhere in the application, but nothing that we'd need to ask InfraEng to update the Terraform docs to remove this env variable.How can a reviewer manually see the effects of these changes?
Run
make test
and verify all unit tests are passing.Includes new or updated dependencies?
YES
Changes expectations for external applications?
NO
What are the relevant tickets?
Developer
Code Reviewer(s)