IN-887 Refactor Carbon app to use new Symplectic FTP parameters #95
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Why these changes are being introduced: * Simplify management of FTP credentials by storing components in a JSON formatted string How this addresses that need: * Expand connection tests for Data Warehouse and Symplectic Elements FTP server * Update CLI tests * Update README Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-887
jonavellecuerdo
force-pushed
the
IN-887-use-new-symplectic-ftp-params
branch
from
e32f063 to
0f2b4ed
Compare
ehanson8
reviewed
Aug 25, 2023
| ```bash | ||
| carbon --help | ||
| ``` | ||
| The Carbon application retrieves 'people' records from the Data Warehouse and generates an XML file that is uploaded to the Symplectic Elements FTP server. On Symplectic Elements, a job is scheduled to ingest the data from the XML file to create user accounts. |
tests/test_cli.py
Outdated
Comment on lines
136
to
144
| monkeypatch.setenv( | ||
| "SYMPLECTIC_FTP_JSON", | ||
| ( | ||
| '{"SYMPLECTIC_FTP_HOST": "localhost", ' | ||
| f'"SYMPLECTIC_FTP_PORT": "{ftp_socket[1]}",' | ||
| '"SYMPLECTIC_FTP_USER": "user", ' | ||
| '"SYMPLECTIC_FTP_PASS": "pass"}' | ||
| ), | ||
| ) |
There was a problem hiding this comment.
This is repeated several times in test_cli, can we make it a fixture? You can leave the block in test_cli_connection_tests_fail since you have to foul up the password
There was a problem hiding this comment.
Please see the latest commit. I used the FTP server fixture when setting up the test environment (i.e.m=, _test_env fixture).
There was a problem hiding this comment.
ftp_socket, ftp_directory = ftp_server
os.environ["FEED_TYPE"] = feed_type
os.environ["SYMPLECTIC_FTP_PATH"] = symplectic_ftp_path
Is there a way to reduce the repetition of this as well?
There was a problem hiding this comment.
Thank you for noting this! I removed these lines of code from the tests so that it could properly use the feed_type and symplectic_ftp_path fixtures.
* Update '_test_env' to use FTP server fixture * Simplify test_load_config_values_success * Fix use of 'feed_type' and 'symplectic_ftp_path' fixtures
jonavellecuerdo
force-pushed
the
IN-887-use-new-symplectic-ftp-params
branch
from
3769d2c to
3377425
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Refactor Carbon app to use new Symplectic FTP SSM parameters
Helpful background context
Prior to these updates, the Carbon app relied on three (3) separate SSM parameters
related to Symplectic Elements FTP credentials:
/tfvars/apps-vars/carbon/ftp-host;/tfvars/apps-vars/carbon/ftp-pass; and/tfvars/apps-vars/carbon/ftp-userTo simplify management of FTP credentials, FTP credentials are now stored
in a one (1) SSM parameter that consists of a single JSON formatted string.
Eventually, the deprecated SSM parameters should be removed to avoid confusion.
Also, @ehanson8 , I added a note about the minor issue related to the CLI test
test_cli_connection_tests_failin theREADME; figured it'd be easier to keep track off than the commit!How can a reviewer manually see the effects of these changes?
If you would like to run it yourself:
IN-887-use-new-symplectic-ftp-params.make install.make lint.make run-connection-tests-stage.Includes new or updated dependencies?
NO
Developer
Code Reviewer
(not just this pull request message)