IN-923 Raise exceptions for failed connection tests / IN-925 Add Makefile commands to run Carbon feeds #111

jonavellecuerdo · 2023-09-20T18:19:19Z

What does this PR do?

A few sentences describing the overall goals of the pull request's commits.
Why are we making these changes? Is there more work to be done to fully
achieve these goals?

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

For IN-923 Raise exceptions for failed connection tests:

Check the CloudWatch for successful connection tests: log
- To produce this log, I ran the Docker image with the code as-is, which should run successfully as it uses the appropriate credentials and does not include a forced ZeroDivisionError.

Check the CloudWatch log for failed connection test for Data Warehouse (tests lines 114-117 of database.py):
- To produce this log, I created a temporary Docker image where I added 1 / 0 after line 113 of carbon.database.DatabaseEngine.run_connection_test() to force a raised exception: log

Check the CloudWatch log for failed connection test for Symplectic Elements FTP server (tests lines 242-247 of app.py):
- To produce this log, I created a temporary Docker image where I added 1 / 0 after line 241 of carbon.app.DatabaseToFtpPipe.run_connection_test() to force a raised exception: log

For IN-925 Add Makefile commands for running 'people' and 'articles' feed as an ECS task:

Check 'people' feed run:

Check the CloudWatch log for successful 'people' feed run: log
- Note: You may see an error message (shown below), but if you check the Elements FTP server, you should see that an XML file still gets uploaded.
See screenshot below showing that HRDataFeed.xml is uploaded to the Elements FTP server (with today's date 9/20):

Check 'articles' feed run:

Check the CloudWatch log for successful 'articles' feed run: log
See screenshot below showing that AA_articles.xml is uploaded to the Elements FTP server (with today's date 9/20):

Note: If you are interested in accessing the Elements FTP server, please refer to the instructions in this Confluence document: Carbon | Developer Guide | How to access files uploaded to the Elements FTP server.

Includes new or updated dependencies?

NO

Developer

README is updated to reflect all changes as needed
All related Jira tickets are linked in commit message(s)

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 to reflect all changes or is not needed
The changes have been verified
New dependencies are appropriate or there were no changes

Why these changes are being introduced: * Raising exceptions for any failed connection tests stops the application from proceeding with the workflow once a connection test fails. This logic makes sense as the (1) a successful connection to the Data Warehouse is required for any Carbon run and (2) a successful connection to the Elements FTP server is required for any run Carbon run that involves FTP. How this addresses that need: * Exceptions are raised in the connection test functions of carbon.database.DatabaseEngine and carbon.app.DatabaseToFtpPipe. * CLI tests for failed connection tests are properly configured. Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-923

Why these changes are being introduced: * Provide an easy way to execute runs of the 'people' and 'articles' feed as an ECS task. How this addresses that need: * Add Makefile commands to run 'people' and 'articles' feed Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-925

ghukill · 2023-09-21T11:55:55Z

Just a couple quick comments as I'm working through the PR:

It'd be helpful to know that your "How can a reviewer manually see the effects of these changes?" section does not require running it per say, but just looking at pre-existing logs.
It'd be helpful to know which environment (e.g. stage) this occurs in! AWS links are often the same across environments, it was only the log group name that indicated it was stage and not dev1.

I was able to quickly piece these together, and can totally see how lost in the shuffle of PR-ing, but just a comment on the PR process of helping the reviewer along for the ride.

That said, the direct cloudwatch log links are helpful, and could see the results.

I think it may come down to a matter of preference, but if the "How can a reviewer manually see the effects..." section is not actually running code, but just observing the output of something somewhat complex to setup, I might propose a 1) link to the lines of code that are getting tested, and 2) screenshot or log snippet of the output. This would avoid the 2 week window and could make the PR process a bit easier. But again, just a thought, and this works as-is.

UPDATE: ...much like you did for the 2nd part of the PR! At a glance, can see from the screenshots that XML files exist. And, with the excellent confluence documentation, can go look at the files myself if needed.

ghukill

Looks good to me!

I left a PR process comment in the PR, that is unrelated from the actual code changes.

I wasn't sure if this was the place to address it, so filed an issue, but I do wonder if the FTP upload error, accompanying a successful upload, may be some kind of hanging file handler and/or open connection? Given the timeout, it feels like it could be something like that.

ghukill · 2023-09-21T12:04:31Z

carbon/app.py

                f"Failed to connect to the Symplectic Elements FTP server: {error}"
            )
            logger.exception(error_message)
+            raise


+1 - think it's good that application bails given a connection error, as it cannot really do its work without it

ghukill · 2023-09-21T12:04:36Z

carbon/database.py

        except Exception as error:
            error_message = f"Failed to connect to the Data Warehouse: {error}"
            logger.exception(error_message)
+            raise


ghukill · 2023-09-21T12:05:25Z

tests/test_cli.py

+def test_cli_database_connection_test_fails(caplog, nonfunctional_engine, runner):
+    with patch("carbon.cli.DatabaseEngine") as mocked_engine:
+        mocked_engine.return_value = nonfunctional_engine
+        result = runner.invoke(main, ["--run_connection_tests"])
+        assert result.exit_code == 1
+
+    assert "Failed to connect to the Data Warehouse" in caplog.text


To me, this is a nice test. The fixture nonfunctional_engine is clearly worded, indicating that it's doomed from the get-go, and the test confirms it.

jonavellecuerdo · 2023-09-21T14:23:13Z

@ghukill I updated the description on the PR above to address your comments (for future reference)! I appreciate your feedback. Regarding the 'non-errors', thank you for looking into it. I will take your thoughts into consideration for the next story which will focus on resolving this issue: [IN-924 Investigate error with custom method for running FTP file transfer](Investigate error with custom method for running FTP file transfer).

ghukill

Looks good! Thanks for the PR updates.

jonavellecuerdo self-assigned this Sep 20, 2023

jonavellecuerdo changed the title ~~IN-923 Raise exceptions for failed connection tests~~ IN-923 Raise exceptions for failed connection tests / IN-925 Add Makefile commands to run Carbon feeds Sep 20, 2023

jonavellecuerdo requested a review from ghukill September 20, 2023 19:06

jonavellecuerdo marked this pull request as ready for review September 20, 2023 19:07

ghukill approved these changes Sep 21, 2023

View reviewed changes

ghukill mentioned this pull request Sep 21, 2023

ftplib error on successful carbon run #112

Open

ghukill self-requested a review September 21, 2023 14:31

ghukill approved these changes Sep 21, 2023

View reviewed changes

jonavellecuerdo merged commit b639b20 into main Sep 21, 2023

jonavellecuerdo deleted the IN-923-925-raise-exceptions-and-add-make-commands branch September 21, 2023 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

IN-923 Raise exceptions for failed connection tests / IN-925 Add Makefile commands to run Carbon feeds #111

IN-923 Raise exceptions for failed connection tests / IN-925 Add Makefile commands to run Carbon feeds #111

Uh oh!

jonavellecuerdo commented Sep 20, 2023 •

edited

Loading

Uh oh!

ghukill commented Sep 21, 2023 •

edited

Loading

Uh oh!

ghukill left a comment •

edited

Loading

Uh oh!

ghukill Sep 21, 2023

Uh oh!

ghukill Sep 21, 2023

Uh oh!

ghukill Sep 21, 2023

Uh oh!

jonavellecuerdo commented Sep 21, 2023

Uh oh!

ghukill left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IN-923 Raise exceptions for failed connection tests / IN-925 Add Makefile commands to run Carbon feeds #111

IN-923 Raise exceptions for failed connection tests / IN-925 Add Makefile commands to run Carbon feeds #111

Uh oh!

Conversation

jonavellecuerdo commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

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

Includes new or updated dependencies?

Developer

Code Reviewer

Uh oh!

ghukill commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghukill left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghukill Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

ghukill Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

ghukill Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

jonavellecuerdo commented Sep 21, 2023

Uh oh!

ghukill left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jonavellecuerdo commented Sep 20, 2023 •

edited

Loading

ghukill commented Sep 21, 2023 •

edited

Loading

ghukill left a comment •

edited

Loading