Conversation
* Add conftest.py and fixtures * Update Makefile * Add new dependencies to Pipfile * Add functionality to get files from S3, unzip them, and return a processed file count * Update setup.cfg to manage mypy errors for new dependencies * Add unit tests for expected data scenarios for new functions * https://mitlibraries.atlassian.net/browse/ENSY-69
hakbailey
left a comment
hakbailey
left a comment
There was a problem hiding this comment.
See inline comments for changes. Happy to chat about any of this!
hakbailey
left a comment
hakbailey
left a comment
There was a problem hiding this comment.
See inline comments for specific changes, plus a couple of general comments:
First, we definitely need to be able to verify this locally. After thinking through it, I think moto_server is a better strategy than Dev1 because if we use the Dev1 s3 bucket someone will have to go into the bucket and find a current prefix every time they submit a PR (because there will be a retention policy on the bucket, so files will get deleted after a period of time). And if it's been long enough that there are no files in the Dev1 bucket (because we won't be running the export from Alma Sandbox regularly going forward), then verification would require first running the publishing job from Alma, which not everyone has permissions to do. So now is probably a good time to go ahead and document how to use moto_server and the fixtures to verify this.
And second, it was ok for the initial structural PR but going forward the commit messages should follow our standard template. PR comments don't show up in the git log, so it's important that the commit messages are detailed including the why, side effects, Jira tickets, etc. The template is used in a lot of commits across repos, but for reference my template looks like this:
########50 characters############################
# Subject
# Summarize changes in around 50 characters or
# less. Capitalize, use imperative, do not end
# with a period.
########72 characters##################################################
# Why these changes are being introduced:
# Problem, Task, Reason for Commit
# How this addresses that need:
# Paragraph or bullet list of changes with reasons
# *
# Side effects of this change:
# None, but adding author info to the new and edit view will need to be
# implemented separately (currently documented in ETD-177, linked above).
# Relevant ticket(s):
# * https://mitlibraries.atlassian.net/browse/[#]
# Resolves: #123 (GH issue)
# See also: #456, #789
Why these changes are being introduced: * Updates based on comments in PR #2 How this addresses that need: * Simplify mocked_s3 fixture bucket with many files for quicker load times * Add BUCKET to test_env fixture * Remove default None value for prefix in filter_files_in_bucket * Remove unnecessary monkeypatch calls in unit tests * Shift bucket with many files fixture to filter_files_in_bucket unit test Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/ENSY-69
|
I updated the commit message style, this template makes perfect sense for my future planned commits but let me know if there's a better way to use this for the assorted edits that come through in a PR review than what I did here |
|
The moto server testing work will be spun off as a different ticket per a conversation with @hakbailey |
I think for PR updates it's fine to just list the changes. In general it seems like we've kind of moved away from squashing commits before merging, but if we went back to doing that then you'd just have one commit for the whole PR, following the template, and the specific changes made during review wouldn't need to be documented. |
|
That makes sense, thanks! |
hakbailey
left a comment
hakbailey
left a comment
There was a problem hiding this comment.
Just a few more small changes and this will be good to go!
* Update Makefile command * Update string formatting of logger message * Remove unnecessary calls of monkeypatch fixture * Update README.md to reflect incomplete functionality
2 participants
Helpful background context
As we discussed offline, I'm trying to consolidate everything in the
ppod.pyfile to streamline this codeHow can a reviewer manually see the effects of these changes?
Let's talk offline about how we want to do this going forward since this now has more testing dependencies than
alma-webhook-lambdas. I'm not sure if it makes more sense to spin up amotoserver and add file(s) for reviewing each PR or whether it makes more sense to take advantage of our dev infrastructure.What are the relevant tickets?
Developer
Code Reviewer
(not just this pull request message)
Includes new or updated dependencies?
YES