-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add GitHub Action to enforce architect approval for strict version pins #44117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
2f19d56
8cc3634
d0e5840
c2684be
45517b7
67cc772
a14b5c9
ee75f22
92f07da
f93a1ae
044c899
79fd49b
fdc7cc2
4addcdd
53b46af
8d799b9
2a41637
d98f9ff
02a98bb
e08dca5
73a5920
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,148 @@ | ||||||||
| # Strict Version Pin Check Workflow | ||||||||
|
|
||||||||
| ## Overview | ||||||||
|
|
||||||||
| This GitHub Actions workflow enforces a policy that requires architect approval for any pull requests that introduce strict version pins (`==`) in main runtime dependencies of Python packages within the `sdk/` directory. | ||||||||
|
|
||||||||
| ## Purpose | ||||||||
|
|
||||||||
| Strict version pins can cause dependency conflicts and limit flexibility in package management. This workflow ensures that any such pins in main runtime dependencies are reviewed and approved by designated architects before merging. | ||||||||
|
|
||||||||
| ## How It Works | ||||||||
|
|
||||||||
| ### Trigger | ||||||||
| The workflow runs on pull requests that modify: | ||||||||
| - `sdk/**/setup.py` | ||||||||
| - `sdk/**/pyproject.toml` | ||||||||
|
|
||||||||
| ### Detection Logic | ||||||||
|
|
||||||||
| The workflow analyzes the diff to detect: | ||||||||
| - **New strict version pins**: Dependencies newly added with `==` operator | ||||||||
| - **Modified pins**: Dependencies changed from flexible constraints (e.g., `>=`, `~=`) to strict `==` pins | ||||||||
|
|
||||||||
| The detection is **scope-aware** and only considers: | ||||||||
| - `install_requires` in `setup.py` | ||||||||
| - `dependencies` under `[project]` in `pyproject.toml` | ||||||||
|
|
||||||||
| The following are **ignored**: | ||||||||
| - Dev dependencies (`extras_require`, `[project.optional-dependencies]`) | ||||||||
| - Test dependencies (`tests_require`) | ||||||||
| - Comments | ||||||||
| - Build dependencies | ||||||||
|
|
||||||||
| ### Approval Requirements | ||||||||
|
|
||||||||
| If strict version pins are detected, the workflow: | ||||||||
| 1. Posts a comment on the PR listing the detected pins | ||||||||
| 2. Checks if any of the designated architects have approved the PR | ||||||||
| 3. **Blocks merging** if no architect approval is found (workflow fails with exit code 1) | ||||||||
| 4. **Allows merging** if an architect has approved | ||||||||
|
|
||||||||
| ### CODEOWNERS Integration | ||||||||
|
|
||||||||
| The `.github/CODEOWNERS` file has been updated to require reviews from the architects for: | ||||||||
| - `/sdk/**/setup.py` | ||||||||
| - `/sdk/**/pyproject.toml` | ||||||||
|
|
||||||||
| This provides a secondary enforcement mechanism through branch protection rules. | ||||||||
|
|
||||||||
| ## Examples | ||||||||
|
|
||||||||
| ### ✅ Allowed (No Strict Pins) | ||||||||
| ```python | ||||||||
| # setup.py | ||||||||
| install_requires=[ | ||||||||
| "azure-core>=1.30.0", | ||||||||
| "requests>=2.21.0", | ||||||||
| ] | ||||||||
| ``` | ||||||||
|
|
||||||||
| ### ⚠️ Requires Architect Approval | ||||||||
| ```python | ||||||||
| # setup.py | ||||||||
| install_requires=[ | ||||||||
| "azure-core==1.30.0", # Strict pin detected! | ||||||||
| "requests>=2.21.0", | ||||||||
| ] | ||||||||
| ``` | ||||||||
|
|
||||||||
| ### ✅ Allowed (Strict Pin in Dev Dependencies) | ||||||||
| ```python | ||||||||
| # setup.py | ||||||||
| install_requires=[ | ||||||||
| "azure-core>=1.30.0", | ||||||||
| ], | ||||||||
| extras_require={ | ||||||||
| "dev": ["pytest==7.0.0"] # OK - this is a dev dependency | ||||||||
| } | ||||||||
| ``` | ||||||||
|
|
||||||||
| ## Testing | ||||||||
|
|
||||||||
| The detection logic has been validated with comprehensive test cases covering: | ||||||||
| - Adding new strict pins | ||||||||
| - Changing from flexible to strict constraints | ||||||||
| - Ignoring dev/test dependencies | ||||||||
| - Ignoring optional dependencies in pyproject.toml | ||||||||
|
|
||||||||
| Run tests locally: | ||||||||
| ```bash | ||||||||
| python /tmp/test_strict_pins.py | ||||||||
| ``` | ||||||||
|
|
||||||||
| ## Files Modified | ||||||||
|
|
||||||||
| 1. **`.github/workflows/check-strict-version-pins.yml`** | ||||||||
| - Main workflow definition | ||||||||
| - Triggers on PR events | ||||||||
| - Runs detection and enforcement logic | ||||||||
|
|
||||||||
| 2. **`.github/scripts/check_strict_pins.py`** | ||||||||
| - Python script that analyzes git diffs | ||||||||
| - Detects strict version pins in appropriate sections | ||||||||
| - Checks for architect approvals via GitHub API | ||||||||
|
|
||||||||
| 3. **`.github/CODEOWNERS`** | ||||||||
| - Added architect requirements for setup.py and pyproject.toml | ||||||||
|
|
||||||||
|
||||||||
| 3. **`.github/CODEOWNERS`** | |
| - Added architect requirements for setup.py and pyproject.toml |
Outdated
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instruction to "Require review from code owners" is misleading since CODEOWNERS is not being used by this implementation. The workflow enforces approval directly through the GitHub API. Consider removing this line or clarifying that code owner review is not required for this specific feature to function.
| - Require review from code owners | |
| - (Optional) Require review from code owners (not required for this workflow; architect approval is enforced directly via the GitHub API) |
Outdated
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This instruction references updating "CODEOWNERS entries in .github/CODEOWNERS", but CODEOWNERS is not being used by this implementation. The architect list is maintained solely in the Python script. This line should be removed or clarified that CODEOWNERS is not used for this feature.
| 2. Update the CODEOWNERS entries in `.github/CODEOWNERS` | |
| 3. Update documentation in this README | |
| 2. Update documentation in this README |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section claims that
.github/CODEOWNERShas been updated, but according to the PR description, CODEOWNERS entries were removed (not added) because they're not needed since the CI enforces approval. This documentation is inconsistent with the actual implementation.This entire "CODEOWNERS Integration" section should be removed or rewritten to clarify that CODEOWNERS is NOT being used for this enforcement. The CI workflow itself handles the approval checks.