Conversation
Why these changes are being introduced: * Add utils module with business logic functions and an SSM client How this addresses that need: * Add parse_accession_number and parse_extent_data functions * Add SSMClient with methods to get and update parameters * Add corresponding unit tests and fixtures for new methods and functions Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/INFRA-367
ghukill
left a comment
ghukill
left a comment
There was a problem hiding this comment.
Overall, looking good! Appreciating the tight tests to new functionality.
Had one question about the SSMClient.update_parameter() method.
| def test_parse_accession_number_4_parts_success(): | ||
| accession_record = { | ||
| "id_0": "2025", | ||
| "id_1": "214", | ||
| "id_2": "314", | ||
| "id_3": "514", | ||
| } | ||
| assert parse_accession_number(accession_record) == "2025-214-314-514" | ||
|
|
||
|
|
||
| def test_parse_accession_number_1_part_success(): | ||
| assert parse_accession_number({"id_0": "2025"}) == "2025" | ||
|
|
||
|
|
||
| def test_parse_extent_data_no_extents_success(): | ||
| assert parse_extent_data({}) == {} |
There was a problem hiding this comment.
These are nice tests. I had some initial questions about the terse nature of the utility function, but these answered them!
| def test_parse_extent_data_physical_linear_feet_success(): | ||
| accession_record = {"extents": [{"number": "1.0", "extent_type": "linear_feet"}]} | ||
| assert parse_extent_data(accession_record) == { | ||
| "Pre-accessioning extent (linear feet)": 1.0 | ||
| } | ||
|
|
||
|
|
||
| def test_parse_extent_data_digital_gigabyes_success(): | ||
| accession_record = {"extents": [{"number": "2.0", "extent_type": "gigabytes"}]} | ||
| assert parse_extent_data(accession_record) == {"Pre-accessioning extent (GB)": 2.0} | ||
|
|
||
|
|
||
| def test_parse_extent_data_physical_not_linear_feet_success(): | ||
| accession_record = {"extents": [{"number": "12.0", "extent_type": "box(es)"}]} | ||
| assert parse_extent_data(accession_record) == { | ||
| "Extent Number": 12.0, | ||
| "Extent Type": "box(es)", | ||
| } | ||
|
|
||
|
|
||
| def test_parse_extent_data_digital_not_gigabytes_success(): | ||
| accession_record = {"extents": [{"number": "100.0", "extent_type": "megabytes"}]} | ||
| assert parse_extent_data(accession_record) == { | ||
| "Extent Number": 100.0, | ||
| "Extent Type": "megabytes", | ||
| } |
There was a problem hiding this comment.
Same for these tests; nice and focused, they really paint a picture of how the utility function works.
Going forward with this app, if these tests grow, it might be worth considering refactoring to a single, parameterized test... but that feels like a balance. As-is, I like it.
| def update_parameter(self, parameter_name: str, parameter_value: str) -> str: | ||
| self.client.put_parameter( | ||
| Name=parameter_name, | ||
| Value=parameter_value, | ||
| Overwrite=True, | ||
| ) | ||
| updated_parameter = self.client.get_parameter(Name=parameter_name) | ||
| updated_parameter_value = updated_parameter["Parameter"]["Value"] | ||
| logger.info(f"SSM parameter updated: {updated_parameter_value}") | ||
| return updated_parameter_value |
There was a problem hiding this comment.
I'm a little confused here. Am I following that this:
- sets a parameter with
parameter_value, overwriting if exists - retrieves that same parameter
- extracts the value from the boto3 response
- returns the extracted value, which theoretically should equal the original
parameter_valuepassed to this method
Is this to confirm that the passed parameter_value was successfully set, confirmed by immediately retrieving it? If so, would it be worth a check that parameter_value == updated_parameter? Or, if not needed, could you perhaps immediately return the result of self.client.put_parameter()?
There was a problem hiding this comment.
Unfortunately, the put_parameter response very unhelpfully does not include the new value so that is why I added the get_parameter call. But you are correct that I should be checking parameter_value == updated_parameter, I will add that, good catch!
* Add check to verify that SSM parameter is updated to the expected value
Pull Request Test Coverage Report for Build 11899084760Details
💛 - Coveralls |
| if parameter_value == updated_parameter_value: | ||
| logger.info(f"SSM parameter updated: '{updated_parameter_value}'") | ||
| else: | ||
| message = "SSM parameter update failed: '{parameter_value}' was not set" | ||
| raise RuntimeError(message) |
4 participants
Purpose and background context
Creating new app for integrating ArchivesSpace and Airtable. Minimal business logic and an SSM client to start. The bulk of the line changes are coming from
Pipfile.lockHow can a reviewer manually see the effects of these changes?
NA
Includes new or updated dependencies?
YES
Changes expectations for external applications?
NO
What are the relevant tickets?
Developer
Code Reviewer(s)