IN-1676 - Update Makefile, pre-commit, linting, and dependency conventions#22
IN-1676 - Update Makefile, pre-commit, linting, and dependency conventions#22
Conversation
Why these changes are being introduced: These changes encapsulate some discussions to update some devops conventions that operate at the intersection of the Makefile, pre-commit hooks, linting, and dependency handling. These changes were prompted by a critical mass of some structure creep over time, and some feedback of unhelpful friction in some areas. How this addresses that need: * linting section reworked in Makefile * pre-commit fires only on git push * pre-commit no longer runs commands that modify code * `ruff format` is used in place of `black` * vulernability scanning moved to `make security` * security scanning removed from linting and pre-commit hooks Side effects of this change: * Vulnerability scanning is an ad-hoc decision by developers in the short-term, with notifications in Github via dependabot. Longer term, we will rely on renovate for rolling dependency updates. * Local commits are no longer constrained by pre-commit hooks, but hooks are enforced when pushing to Github. * We move further into the ecosystem of astral's ruff library, using the built-in formatter instead of black. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1676
e09512f to
d0455a4
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates developer tooling conventions around formatting/linting, pre-commit hook staging, and security scanning to reduce local friction while keeping push-time enforcement.
Changes:
- Remove
blackfrom dev dependencies and rely onruff formatinstead. - Rework Makefile linting targets and move vulnerability scanning to
make security. - Update pre-commit configuration to run hooks on
pre-pushand avoid code-modifying hooks.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
uv.lock |
Removes black and updates/realigns dev tooling dependency graph (e.g., ruff, coveralls). |
pyproject.toml |
Drops black from the dev dependency group. |
Makefile |
Reworks linting/security targets and changes pre-commit hook installation behavior. |
.pre-commit-config.yaml |
Defaults hooks to the pre-push stage and switches formatting to ruff format --diff. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Per the CoPilot review suggestions above, added a new commit that decouples the pre-commit commit and push hooks. It was a nice catch: I had forgotten that this It's actually checking the state of the filepath Confirmed that |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pyproject.toml:23
blackwas removed from the dev dependency group, but the project still contains a[tool.black]configuration section below. To avoid confusion (and to keep formatter configuration in one place), consider removing[tool.black]or translating any needed settings (e.g. line length) into Ruff’s formatter configuration.
[dependency-groups]
dev = [
"coveralls>=4.0.1",
"mypy>=1.17.1",
"pip-audit>=2.9.0",
"pre-commit>=4.3.0",
"pytest>=8.4.1",
"ruff>=0.12.8",
]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| install: .venv .git/hooks/pre-commit .git/hooks/pre-push # Install Python dependencies and create virtual environment if not exists | ||
| uv sync --dev | ||
|
|
||
| .venv: # Creates virtual environment if not found | ||
| @echo "Creating virtual environment at .venv..." | ||
| uv venv .venv | ||
|
|
||
| .git/hooks/pre-commit: # Sets up pre-commit hook if not setup | ||
| @echo "Installing pre-commit hooks..." | ||
| uv run pre-commit install | ||
| .git/hooks/pre-commit: # Sets up pre-commit commit hooks if not setup | ||
| @echo "Installing pre-commit commit hooks..." | ||
| uv run pre-commit install --hook-type pre-commit | ||
|
|
||
| .git/hooks/pre-push: # Sets up pre-commit push hooks if not setup | ||
| @echo "Installing pre-commit push hooks..." | ||
| uv run pre-commit install --hook-type pre-push |
There was a problem hiding this comment.
The install recipe still installs a pre-commit hook (in addition to pre-push), but .pre-commit-config.yaml sets default_stages: [pre-push], so the commit-time hook will be a no-op (and contradicts the stated goal of “hooks only on push”). Consider removing .git/hooks/pre-commit from install (and the corresponding target), or expanding default_stages to include pre-commit if you do want commit-time enforcement.
There was a problem hiding this comment.
Noted, but we want the ability to modify the default_stages in .pre-commit-config.yaml and have the hooks ready.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ehanson8
left a comment
There was a problem hiding this comment.
Great work, looking forward to reorienting our workflows more explicitly towards this as the source of truth!
jonavellecuerdo
left a comment
There was a problem hiding this comment.
This looks great, and I found it helpful to read the review and discussions with GitHub Copilot in the Conversation tab. Just one tiny question for ya'!
| @@ -1,4 +1,5 @@ | |||
| name: CI | |||
| permissions: read-all | |||
There was a problem hiding this comment.
I was curious about this change. 🤔 I don't recall it in any of the docs for proposed updates. From a quick search, I see that this config is used to "modify permissions for the GITHUB_TOKEN for an entire workflow or for individual jobs."
Just confirming that this is the setting we require. From docs, I pulled:
- As a good security practice, you should always make sure that actions only have the minimum access they require by limiting the permissions granted to the
GITHUB_TOKEN. (source)
There was a problem hiding this comment.
Nice catch! This was an explicit request from Christopher.
Purpose and background context
Why these changes are being introduced:
These changes encapsulate some discussions to update some devops conventions that operate at the intersection of the Makefile, pre-commit hooks, linting, and dependency handling. These changes were prompted by a critical mass of some structure creep over time, and some feedback of unhelpful friction in some areas.
How this addresses that need:
ruff formatis used in place ofblackmake securitySide effects of this change:
NOTE: For the pre-commit changes to take effect, it's recommended to run
make installwhich will re-run pre-commit installations and settings.Includes new or updated dependencies?
YES
Changes expectations for external applications?
NO
What are the relevant tickets?
Code review