Add GitHub Action to enforce architect approval for strict version pins #44117
Conversation
…tect approval Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com>
Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com>
|
@copilot help me fix the CI errors |
… of workflows directory Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com>
Fixed the CI errors in commit c2684be:
The workflow should now pass CI checks. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot remove the codeowner names from the readme |
Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com>
Removed the codeowner names from the README in commit 45517b7. The documentation now refers to "designated architects" instead of listing specific usernames. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot ignoring the spelling errors in the python file |
Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com>
Added Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
/azp run python - eventgrid |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
@copilot Can we make sure that we are writing clean pythonic code and lets fix the spelling issues or by file ignore them rather than cpsell ignoring the whole scripts path |
There was a problem hiding this comment.
Pull request overview
This PR introduces a GitHub Actions workflow that enforces architect approval for pull requests introducing strict version pins (==) in main runtime dependencies within the Azure SDK for Python repository. The workflow analyzes diffs in setup.py and pyproject.toml files, detects new strict pins, and blocks merging unless approved by designated architects (kashifkhan, annatisch, or johanste).
Key Changes:
- Adds automated detection workflow for strict version pins in Python dependencies
- Implements diff-based analysis to identify only newly introduced pins
- Provides clear PR comments indicating approval requirements
- Blocks merge via workflow failure when approval is missing
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
.github/workflows/check-strict-version-pins.yml |
Main workflow that triggers on PR changes to setup.py/pyproject.toml files, runs detection script, posts comments, and blocks merge if needed |
.github/scripts/check_strict_pins.py |
Python script that analyzes git diffs to detect strict version pins in install_requires/dependencies sections while ignoring dev/test dependencies |
.github/README-strict-version-pins.md |
Documentation explaining the workflow's purpose, behavior, and maintenance procedures (contains inaccuracies about CODEOWNERS) |
| 2. Update the CODEOWNERS entries in `.github/CODEOWNERS` | ||
| 3. Update documentation in this README |
There was a problem hiding this comment.
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 |
| if bracket_depth <= 0 and (']' in actual_line or '),' in actual_line): | ||
| # Check current line before exiting if it's an added line | ||
| if is_added and '==' in actual_line and not actual_line.startswith('#'): | ||
| match = re.search(r'["\']([^"\']+==[\d.]+[^"\']*)["\']', actual_line) |
There was a problem hiding this comment.
Same issue as line 103 - the regex pattern will fail to match version specifiers with letters in them (pre-release, post-release, dev versions, etc.). Consider using a more comprehensive pattern to capture all valid PEP 440 version specifiers.
| def get_changed_files(base_ref: str, head_ref: str) -> List[str]: | ||
| """Get list of changed setup.py and pyproject.toml files in sdk directory.""" | ||
| diff_output = run_git_command([ | ||
| "git", "diff", "--name-only", "--diff-filter=AM", |
There was a problem hiding this comment.
The --diff-filter=AM flag only considers Added and Modified files, but excludes Renamed files (R). If a file is renamed that contains strict version pins, those changes would not be detected. Consider adding R to the filter (i.e., --diff-filter=AMR) to handle renamed files that may also contain modifications.
| "git", "diff", "--name-only", "--diff-filter=AM", | |
| "git", "diff", "--name-only", "--diff-filter=AMR", |
There was a problem hiding this comment.
For the purposes of the functionality of this PR, I disagree.
| import sys | ||
| import urllib.request | ||
| import urllib.parse | ||
| from typing import Dict, List, Optional |
There was a problem hiding this comment.
The Optional type from typing is imported but never used in the code. Consider removing this unused import for cleaner code.
| from typing import Dict, List, Optional | |
| from typing import Dict, List |
|
|
||
| # Detect [project] section markers in any line (context or changes) | ||
| if actual_line.startswith('['): | ||
| if actual_line.startswith('[project]'): |
There was a problem hiding this comment.
The logic for detecting TOML sections has a bug. On line 146, it checks if actual_line.startswith('[project]'), but this will fail to match [project] sections that have trailing content or are nested (e.g., [project.optional-dependencies]).
The current logic will incorrectly treat [project.optional-dependencies] as "not other section" because it starts with [project, but then it's supposed to be filtered out by line 154. However, if a line like [project.something-else] appears, it would be incorrectly treated as the main [project] section.
Consider changing line 146 to an exact match: if actual_line == '[project]': to ensure you're only matching the main [project] section header, not subsections.
| if actual_line.startswith('[project]'): | |
| if actual_line == '[project]': |
| pull_request: | ||
| paths: | ||
| - 'sdk/**/setup.py' | ||
| - 'sdk/**/pyproject.toml' |
There was a problem hiding this comment.
[nitpick] The workflow will also trigger for changes to this workflow file itself (.github/workflows/check-strict-version-pins.yml) if a setup.py or pyproject.toml file exists at that path, because the glob pattern sdk/**/setup.py could theoretically match files anywhere under sdk/. However, this is likely not an issue in practice.
A more significant concern is that the workflow will trigger even if only .github/workflows/check-strict-version-pins.yml or .github/scripts/check_strict_pins.py are modified, if those changes happen to be in a PR that also touches other files. Consider whether you want to add those workflow/script paths to the trigger paths so the workflow can be tested when modified.
| - 'sdk/**/pyproject.toml' | |
| - 'sdk/**/pyproject.toml' | |
| - '.github/workflows/check-strict-version-pins.yml' | |
| - '.github/scripts/check_strict_pins.py' |
| 3. **`.github/CODEOWNERS`** | ||
| - Added architect requirements for setup.py and pyproject.toml | ||
|
|
There was a problem hiding this comment.
This lists .github/CODEOWNERS as a file that was modified with "Added architect requirements for setup.py and pyproject.toml", but this is incorrect. According to the PR description, CODEOWNERS entries were removed, and this file was not actually changed in the PR. This should be removed from the Files Modified section.
| 3. **`.github/CODEOWNERS`** | |
| - Added architect requirements for setup.py and pyproject.toml |
| if in_install_requires and is_added: | ||
| if '==' in actual_line and not actual_line.startswith('#'): | ||
| # Match package==version pattern | ||
| match = re.search(r'["\']([^"\']+==[\d.]+[^"\']*)["\']', actual_line) |
There was a problem hiding this comment.
The regex pattern r'["\']([^"\']+==[\d.]+[^"\']*)["\']' may not correctly match all valid version specifiers. Specifically, [\d.]+ only matches digits and dots, which excludes:
- Pre-release versions (e.g.,
package==1.0.0a1,package==1.0.0rc1) - Post-release versions (e.g.,
package==1.0.0.post1) - Dev versions (e.g.,
package==1.0.0.dev1) - Version strings with letters in them
Consider using a more comprehensive pattern like [\w.]+ or [^"\']+ (since you already have [^"\']* after the version) to capture all valid PEP 440 version specifiers.
| bracket_depth = 0 | ||
| # Check if the array starts on the same line | ||
| if '[' in actual_line: | ||
| bracket_depth = actual_line.count('[') - actual_line.count(']') | ||
|
|
||
| # Detect end of install_requires or start of other sections | ||
| if in_install_requires: | ||
| if 'extras_require' in actual_line or 'tests_require' in actual_line: | ||
| in_install_requires = False | ||
| continue | ||
|
|
||
| # Track brackets in all lines | ||
| bracket_depth += actual_line.count('[') - actual_line.count(']') |
There was a problem hiding this comment.
The bracket depth tracking logic has a potential issue when install_requires is detected on a line. On line 85, bracket_depth is reset to 0, but then on line 88, the bracket depth is calculated by counting brackets on the install_requires line. However, on line 97, brackets are counted again for all lines in the install_requires section, which could lead to double-counting if we've already counted brackets on line 88 for the same line.
Consider restructuring this to avoid double-counting: either don't count brackets on the install_requires line initially, or skip the bracket counting on line 97 for the first iteration after detecting install_requires.
| let message = '## ⚠️ Strict Version Pin Detected\n\n'; | ||
| message += 'This PR introduces one or more strict version pins (`==`) in main runtime dependencies:\n\n'; | ||
| message += '```\n' + strictPins + '\n```\n\n'; |
There was a problem hiding this comment.
The strictPins variable is directly interpolated into the GitHub comment without any sanitization or escaping. While the Python script does escape special characters for GitHub Actions output (%, \n, \r), it doesn't escape characters that could break the markdown formatting or potentially be used for injection in the GitHub comment context.
For example, if a dependency name contained backticks or other markdown special characters, it could break out of the code block. Consider adding additional sanitization or escaping in the JavaScript code before inserting into the message, or validate/sanitize the dependency names in the Python script.
| let message = '## ⚠️ Strict Version Pin Detected\n\n'; | |
| message += 'This PR introduces one or more strict version pins (`==`) in main runtime dependencies:\n\n'; | |
| message += '```\n' + strictPins + '\n```\n\n'; | |
| // Escape backticks and triple backticks to prevent breaking markdown code block | |
| function escapeMarkdownCodeBlock(str) { | |
| // Replace triple backticks with single backticks and escape single backticks | |
| return str.replace(/```/g, "\\`\\`\\`").replace(/`/g, "\\`"); | |
| } | |
| const safeStrictPins = escapeMarkdownCodeBlock(strictPins); | |
| let message = '## ⚠️ Strict Version Pin Detected\n\n'; | |
| message += 'This PR introduces one or more strict version pins (`==`) in main runtime dependencies:\n\n'; | |
| message += '```\n' + safeStrictPins + '\n```\n\n'; |
| name: Check for Strict Version Pins | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
| pull_request: | |
| pull_request_target: |
When adding a workflow that will run with elevated permissions, we must use pull_request_target so that the PR cannot ALSO include changes to this workflow that could be utilized in ways we don't intend.
| return files | ||
|
|
||
|
|
||
| def extract_strict_pins_from_setup_py_diff(diff_content: str) -> List[str]: |
There was a problem hiding this comment.
Install eng/tools/azure-sdk-tools and use it to parse the directory of the changed pyproject.toml or setup.py. using ParsedSetup.from_path(). It should handle all edge cases. You don't need any of this extra code IMO.
| url = f"https://api.github.com/repos/{repo}/pulls/{pr_number}/reviews" | ||
|
|
||
| headers = { | ||
| 'Authorization': f'token {github_token}', |
There was a problem hiding this comment.
If you bring in azure-sdk-tools you can also install the ghtools extra and get access to pygithub so you don't need to formulate the request yourself.
scbedd
left a comment
scbedd
left a comment
There was a problem hiding this comment.
We are writing duplicate parsing code + we have an unsafe action configuration. Please take a look! I won't fight the python script too hard, but we definitely should adjust the action yaml config. We'll be compromised if we merge this most likely.
|
Why are we trying to put this into a github action as opposed to the existing DevOPs pipeline we have? |
I am assuming we want it to be run against PRs and be able to block PRs while not giving access to the larger |
I only noticed this PR because My initial feedback, this looks like a lot of code (300 lines Python, 100 lines YML) for what should be a simple check conceptually (require extra approvers on PRs pinning versions). The exact minimal check you want might require this much code, but maybe you could get close enough with 10x less code, using an existing mechanism like Also, if this were the specs repo:
|
For GH Action vs DevOps Pipeline -- no particular reason looking at it now, at the time I was thinking visually I wanted the user to see architect approval was required, which I believe I can do with the pipeline too (and then the script can stay in python?) Yes will take a look at the code and try CODEOWNERS again |
l0lawrence
requested review from
annatisch,
benbp,
iscai-msft,
johanste,
lmazuel,
mccoyp,
pvaneck,
weshaggard and
xiangyan99
as code owners
|
going to reopen |
Implementation Complete ✅
/sdk/**/setup.pyand/sdk/**/pyproject.tomlRecent Changes:
Fixed cspell issues in all relevant files:
<!-- cspell:ignore pyproject kashifkhan annatisch johanste -->to the README# cspell:ignore pyproject kashifkhan annatisch johansteto the workflow YAMLAll three files that reference technical terms and architect usernames now have file-specific cspell ignore comments.
Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.