-
Notifications
You must be signed in to change notification settings - Fork 2
IN-928 Restructure the config module to use a 'Config' class #121
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
Conversation
Why these changes are being introduced: * Refactor configuration process and improve code readability How this addresses that need: * Create Config class * Update dependent modules to use Config * Update unit tests * Simplify error logging for failed connection tests * Add .dockerignore file Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-928
8104917 to
70cb9c3
Compare
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 think it looks good! Nice work.
All my comments are questions / suggestions, so not formally requesting any changes and consider it approved. If you decide to implement any of these changes happy to re-review, but if you'd prefer to skip, okay with that too!
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.
Looks good, a few questions. And I would recommend making the changes Graham suggested to streamline it. Great work!
db1e48a to
b88fbc4
Compare
What does this PR do?
Refactor config module to use a
Configclass to improve code readability and flow.The CLI
main()method was also updated to exit cleanly in the event of failedconnection tests.
Helpful background context
Updating the CLI
main()methodTracebackwas printing twice in the CloudWatch logs.try-exceptblock;carbon.app.run_all_connection_tests()to keepcli.pyorganized.
oracledb.exceptions.DatabaseError: ORA-12170: TNS:Connect timeout occurred" and posted thatlogging.erroris sufficient for such cases (this requires ignoring Ruff ruleTRY400).carbon.app.helpers, but it wasn't possible due to a circular import.Added a
.dockerignorefile..dockerignorefile from the Python CLI template repo!How can a reviewer manually see the effects of these changes?
Review the following CloudWatch logs in
Stage-Workloads.Log showing clean exit from

carbon.app.run_all_connection_testsand simplified error log for failed Data Warehouse connection test.Log showing clean exit from

carbon.app.run_all_connection_testsand simplified error log for failed Elements FTP connection test.Log showing clean exit from

carbon.app.run_all_connection_testswhere all connection tests were successful.Log showing successful Carbon run for HR Feed using

carbon.config.ConfigLog showing successful Carbon run for Articles Feed using

carbon.config.ConfigScreenshot showing XML files on Elements FTP server.

Includes new or updated dependencies?
NO
Developer
Code Reviewer
(not just this pull request message)