-
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
Config
class 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()
methodTraceback
was printing twice in the CloudWatch logs.try-except
block;carbon.app.run_all_connection_tests()
to keepcli.py
organized.
oracledb.exceptions.DatabaseError: ORA-12170: TNS:Connect timeout occurred
" and posted thatlogging.error
is sufficient for such cases (this requires ignoring Ruff ruleTRY400
).carbon.app.helpers
, but it wasn't possible due to a circular import.Added a
.dockerignore
file..dockerignore
file 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_tests
and simplified error log for failed Data Warehouse connection test.Log showing clean exit from

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

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

carbon.config.Config
Log showing successful Carbon run for Articles Feed using

carbon.config.Config
Screenshot showing XML files on Elements FTP server.

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