Conversation
Why these changes are being introduced: This overhaul of the lambda template has been percolating for some time, with changes like pipenv to uv, use of AWS SAM for testing, and some updates to how we run linting, pre-commit hooks, etc. This work is taken on together to bring a holistic upgrade to the template instead of piecemeal work that was entrenching some stale patterns. How this addresses that need: It was going to be difficult to break this work out into meaningful smaller commits, given how interwoven the changes are. The work breaks down into a few major areas: 1. Update from pipenv to uv 2. Introduce a Config class and simplify the lambda skeleton 3. Introduce AWS SAM for local testing 4. Update linting, pre-commit, and Makefile conventions While theoretically possible to commit those changes in order, it felt like an artifical divvying up of work. These changes have been vetted in other contexts independently; this commit weaves them together for this template repository. Side effects of this change: * uv nows used for python environment and dependencies * pre-commit only fires for git push, not git commit * security scanning is opt-in until renovate becomes a rolling dependency manager in CI Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1542
Pull Request Test Coverage Report for Build 22727370728Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Python Lambda template by switching dependency/tooling management to uv, introducing a centralized Config/logging/Sentry setup, and adding AWS SAM assets and Makefile targets for local testing while updating CI and repository conventions.
Changes:
- Migrate from Pipenv (
Pipfile*) touv(pyproject.toml,uv.lock) and update CI workflows accordingly. - Add
lambdas/config.pyfor configuration, logging, and Sentry initialization; simplify the Lambda skeleton to use it. - Add AWS SAM templates and Makefile commands for local Lambda testing; refresh linting/pre-commit conventions.
Reviewed changes
Copilot reviewed 15 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Adds uv lockfile for pinned dependency resolution. |
pyproject.toml |
Defines project metadata, dependencies, and tool configs for uv/mypy/ruff. |
Pipfile |
Removes Pipenv manifest (migration to uv). |
Pipfile.lock |
Removes Pipenv lockfile (migration to uv). |
Dockerfile |
Updates base image to Python 3.13 and installs deps via uv export/pip install flow. |
.python-version |
Bumps local Python version to 3.13. |
Makefile |
Replaces Pipenv commands with uv, adds lint/security targets, and adds SAM targets. |
.pre-commit-config.yaml |
Moves hooks to uv-based commands and defaults execution to pre-push. |
.github/workflows/ci.yml |
Switches reusable workflows to uv-based shared CI jobs and adjusts trigger. |
.github/pull-request-template.md |
Updates PR checklist to newer dev/reviewer conventions. |
.gitignore |
Ignores SAM build output and local SAM env file. |
lambdas/config.py |
Introduces Config class plus logger and Sentry configuration helpers. |
lambdas/my_function.py |
Refactors lambda module to use centralized config/logging/Sentry setup. |
tests/test_my_function.py |
Simplifies handler test to match updated handler signature. |
tests/test_config.py |
Adds unit tests for Config required-env checks and Sentry configuration behavior. |
tests/sam/template.yaml |
Adds a SAM template for local execution/testing of the image-based Lambda. |
tests/sam/env.json.template |
Adds a template for sensitive env overrides in local SAM runs. |
README.md |
Updates developer workflow docs to uv + AWS SAM usage and refreshed conventions. |
Comments suppressed due to low confidence (1)
README.md:116
SENTRY_DSNis listed under Required env vars, but the description says it is “not needed for local development” andconfigure_sentry()already behaves as optional when it’s unset. Consider movingSENTRY_DSNto the Optional section (or updating the wording) so the documentation matches actual behavior.
### Required
```shell
SENTRY_DSN=### If set to a valid Sentry DSN, enables Sentry exception monitoring. This is not needed for local development.
WORKSPACE=### Set to `dev` for local development, this will be set to `stage` and `prod` in those environments by Terraform.
</details>
---
💡 <a href="/MITLibraries/python-lambda-template/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
lambdas/config.py
Outdated
| "SENTRY_DSN", | ||
| ) | ||
| OPTIONAL_ENV_VARS = ("WARNING_ONLY_LOGGERS",) |
There was a problem hiding this comment.
Config.REQUIRED_ENV_VARS includes SENTRY_DSN, but the rest of the code treats Sentry as optional (e.g., sentry_dsn returns None for unset/"None" and configure_sentry() logs and exits). This also makes Config().check_required_env_vars() disagree with tests/test_config.py (it will report SENTRY_DSN missing). Consider moving SENTRY_DSN to optional env vars (or excluding it from required checks) so required-var validation matches runtime behavior and tests.
| "SENTRY_DSN", | |
| ) | |
| OPTIONAL_ENV_VARS = ("WARNING_ONLY_LOGGERS",) | |
| ) | |
| OPTIONAL_ENV_VARS = ( | |
| "WARNING_ONLY_LOGGERS", | |
| "SENTRY_DSN", | |
| ) |
ehanson8
left a comment
There was a problem hiding this comment.
SAM worked as expected, but 2 minor comments!
.github/pull-request-template.md
Outdated
| ### Developer | ||
| - [ ] All new ENV is documented in README | ||
| - [ ] All new ENV has been added to staging and production environments | ||
| - [ ] All related Jira tickets are linked in commit message(s) | ||
| - [ ] Stakeholder approval has been confirmed (or is not needed) | ||
|
|
||
| ### Code Reviewer(s) | ||
| - [ ] The commit message is clear and follows our guidelines (not just this PR message) | ||
| - [ ] There are appropriate tests covering any new functionality | ||
| - [ ] The provided documentation is sufficient for understanding any new functionality introduced | ||
| - [ ] Any manual tests have been performed and verified | ||
| - [ ] New dependencies are appropriate or there were no changes No newline at end of file |
There was a problem hiding this comment.
I don't think this should be coming back, I'm guessing this was inadvertent
There was a problem hiding this comment.
Nice catch! Thanks. I have updated the 2026-03 spec (again, in the name of this grand experiment): https://mitlibraries.atlassian.net/wiki/spaces/IN/pages/5205491713/2026-03-05+-+Python+Projects+DevOps+Updates#Pull-Request-Template.
Update forthcoming.
There was a problem hiding this comment.
Here is an automated check against the spec now catching this:
### General
- ✅ .python-version = 3.13
- ✅ .aws-architecture = linux/arm64
- ✅ No pipenv references in README
- ✅ .gitignore has .aws-sam/ and tests/sam/env.json
- ❌ .gitignore missing .arch_tag
- ❌ Pull request template doesn't match canonical form (has extra Developer/Reviewer checklists, missing Code review section with link)
And actually, catching something new (the .arch_tag gitignore). That was added to the spec per some updates for other repositories. It's complex, and I think we'll want to talk about order and implications, but I can't shake the feeling it's doing exactly what it could as a specification: things we don't even think/know to look for are caught.
Why these changes are being introduced: There has long been some friction with the SENTRY_DSN env var. In a production setting we ideally do not want an application to get deployed without the SENTRY_DSN set. At the same time, it's really not operationally required for most applications. It's probably the wrong ergonomics to have it checked generally and always at the config level, when it's more of a deployed check. How this addresses that need: * make SENTRY_DSN optional * issues a WARNING log message when unset or explicit "None" value Side effects of this change: * An application could get deployed without a SENTRY_DSN * We can, and should, look at deploy practices that will enforce SENTRY_DSN being present Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1542
Purpose and background context
Why these changes are being introduced:
This overhaul of the lambda template has been percolating for some time, with changes like pipenv to uv, use of AWS SAM for testing, and some updates to how we run linting, pre-commit hooks, etc.
This work is taken on together to bring a holistic upgrade to the template instead of piecemeal work that was entrenching some stale patterns.
How this addresses that need:
It was going to be difficult to break this work out into meaningful smaller commits, given how interwoven the changes are. The work breaks down into a few major areas:
While theoretically possible to commit those changes in order, it felt like an artifical divvying up of work. These changes have been vetted in other contexts independently; this commit weaves them together for this template repository.
Side effects of this change:
How can a reviewer manually see the effects of these changes?
Recommended approach:
pipenvvirtual environmentmake installfor freshuvenvironmentmake test+make lintIncludes new or updated dependencies?
YES
Changes expectations for external applications?
NO
What are the relevant tickets?
Code review