Align with spec-python-projects - #158
Conversation
e5ea28c to
3545331
Compare
Why these changes are being introduced: * Aligning this repo with our current best practices represented by spec-python-projects How this addresses that need: * Various updates produced by running the /mitlib-spec-align-project skills which were reviewed and successfully tested. Side effects of this change: * NA Relevant ticket(s): * NA
c21272e to
54e5f88
Compare
ghukill
left a comment
ghukill
left a comment
There was a problem hiding this comment.
Overall, looks great!
Requested one change in the Makefile that was not introduced by this PR, but seems like a good time to make the update.
With that change, approved! My running of a local audit passes as well (with some understandable warnings).
| patron_dict["OFFICE_ADDRESS"] | ||
| if patron_dict["OFFICE_ADDRESS"] | ||
| else "NO ADDRESS ON FILE IN DATA WAREHOUSE" | ||
| patron_dict["OFFICE_ADDRESS"] or "NO ADDRESS ON FILE IN DATA WAREHOUSE" |
There was a problem hiding this comment.
I remain kind of impressed how ruff linting will pickup stuff like this. Nice update!
| @@ -10,85 +10,135 @@ DATETIME:=$(shell date -u +%Y%m%dT%H%M%SZ) | |||
| S3_BUCKET:=shared-files-$(shell aws sts get-caller-identity --query "Account" --output text) | |||
There was a problem hiding this comment.
I know these code changes didn't introduce this, but I'd propose a change on this line that calls aws CLI.
As I was running make test and make lint, I was getting warnings about my AWS credentials being stale.
If we remove the colon in the assignment, it sounds like it is lazily evaluated (new to me!) and thus doesn't all the aws sts ... command unless the env var is used, which is only used by the make dependencies command.
Updated form:
S3_BUCKET=shared-files-$(shell aws sts get-caller-identity --query "Account" --output text)I tested with a Makefile command like this:
test-lazy-env-vars:
echo $(S3_BUCKET)
- if no credentials set or they are stale, it fails
- if credentials are present, it works
There was a problem hiding this comment.
Good catch, updated!
2 participants
Purpose and background context
Aligning this repo with our current best practices represented by our Python repo spec
How can a reviewer manually see the effects of these changes?
The updated ECR image was pushed to
Stage(Dev1can't connect to the Data Warehouse) and successfully tested on 2026-09-02, Cloudwatch logs.Includes new or updated dependencies?
YES
Changes expectations for external applications?
NO
What are the relevant tickets?
Code review