fix(scripts): add smg-grpc-servicer to version check and fix pyproject.toml support - #683
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical issues in the release version checking script by expanding its coverage to include a previously overlooked gRPC servicer and rectifying an incorrect version extraction method for an existing package. The core change involves enhancing the script's Python version handling functions to robustly support Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughscripts/check_release_versions.sh was extended to be TOML-aware for Python packages: it reads and writes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36aeb10738
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "smg-grpc-proto|crates/grpc_client/python|crates/grpc_client/python/pyproject.toml" | ||
| "smg-grpc-servicer|grpc_servicer|grpc_servicer/pyproject.toml" |
There was a problem hiding this comment.
Switching version_file to pyproject.toml means Phase 1b’s change detector now filters out the package’s entire metadata file (grep -cv "$(basename "$version_file")$"), so a commit that only changes dependencies or other package metadata in pyproject.toml is counted as unchanged and skips the version-bump check. This creates false negatives for both smg-grpc-proto and smg-grpc-servicer, allowing release-impacting Python package changes to land without a required version bump.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly adds smg-grpc-servicer to the version check and switches to using pyproject.toml as the source of truth for Python package versions, which is a solid improvement. My review focuses on making the shell script's parsing logic more robust. I've identified a couple of places where the sed and awk commands could fail on edge cases, such as when version files contain comments. The suggested changes will improve the reliability of version parsing and updating.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/check_release_versions.sh (1)
241-285: 🛠️ Refactor suggestion | 🟠 MajorScope the TOML match to the package metadata section.
The current
.tomlbranch matches the firstversion =line anywhere in the file. While the existingpyproject.tomlfiles in the repository have only oneversionkey (under[project]), the regex pattern is fragile. If a futurepyproject.tomladds aversionkey outside[project]/[tool.poetry](e.g., for documentation or build configuration),get_python_version(),get_python_version_at_ref(), andset_python_version()will all match the wrong field, and the post-write verification inset_python_version()will still pass with the incorrect value.Constrain the match to track the active TOML section and only extract/update
versionwithin[project]or[tool.poetry], or fail explicitly if the expected version key is not found.♻️ Suggested hardening
get_python_version() { local file="$1" if [[ "$file" == *.toml ]]; then - grep -m1 '^version' "$file" | sed 's/.*"\(.*\)".*/\1/' + awk ' + /^\[/ { section=$0 } + (section == "[project]" || section == "[tool.poetry]") && + /^[[:space:]]*version[[:space:]]*=[[:space:]]*".*"$/ { + match($0, /"([^"]+)"/, m) + print m[1] + exit + } + ' "$file" else grep '__version__' "$file" | sed 's/.*"\(.*\)".*/\1/' fi } get_python_version_at_ref() { @@ - if [[ "$file" == *.toml ]]; then - echo "$content" | grep -m1 '^version' | sed 's/.*"\(.*\)".*/\1/' + if [[ "$file" == *.toml ]]; then + echo "$content" | awk ' + /^\[/ { section=$0 } + (section == "[project]" || section == "[tool.poetry]") && + /^[[:space:]]*version[[:space:]]*=[[:space:]]*".*"$/ { + match($0, /"([^"]+)"/, m) + print m[1] + exit + } + ' else echo "$content" | grep '__version__' | sed 's/.*"\(.*\)".*/\1/' fi } set_python_version() { @@ if [[ "$file" == *.toml ]]; then - awk -v new="$new_version" ' - !done && /^version = ".*"/ { sub(/^version = ".*"/, "version = \"" new "\""); done=1 } + awk -v new="$new_version" ' + /^\[/ { section=$0 } + !done && + (section == "[project]" || section == "[tool.poetry]") && + /^[[:space:]]*version[[:space:]]*=[[:space:]]*".*"$/ { + sub(/=.*/, "= \"" new "\"") + done=1 + } { print } + END { exit done ? 0 : 1 } ' "$file" > "${file}.tmp" && mv "${file}.tmp" "$file" - if ! grep -q "^version = \"${new_version}\"" "$file"; then + if ! awk -v expected="$new_version" ' + /^\[/ { section=$0 } + (section == "[project]" || section == "[tool.poetry]") && + $0 ~ "^[[:space:]]*version[[:space:]]*=[[:space:]]*\"" expected "\"$" { found=1 } + END { exit found ? 0 : 1 } + ' "$file"; then echo -e " ${RED}FAILED to update $file${NC}" >&2 return 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check_release_versions.sh` around lines 241 - 285, The TOML handling in get_python_version, get_python_version_at_ref, and set_python_version currently grabs the first version= anywhere; change each to only consider version lines that appear inside the [project] or [tool.poetry] sections: when reading (get_python_version and get_python_version_at_ref) parse the file/content by tracking the current TOML section and emit the version only if the active section is [project] or [tool.poetry], returning empty/failing if none found; when writing (set_python_version) update the version line only if found inside those sections (use the same section-aware scan/update) and error/return non‑zero if no matching version key in the allowed sections was updated so the verification check is meaningful. Ensure references to the functions get_python_version, get_python_version_at_ref, and set_python_version are used to locate edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/check_release_versions.sh`:
- Around line 241-285: The TOML handling in get_python_version,
get_python_version_at_ref, and set_python_version currently grabs the first
version= anywhere; change each to only consider version lines that appear inside
the [project] or [tool.poetry] sections: when reading (get_python_version and
get_python_version_at_ref) parse the file/content by tracking the current TOML
section and emit the version only if the active section is [project] or
[tool.poetry], returning empty/failing if none found; when writing
(set_python_version) update the version line only if found inside those sections
(use the same section-aware scan/update) and error/return non‑zero if no
matching version key in the allowed sections was updated so the verification
check is meaningful. Ensure references to the functions get_python_version,
get_python_version_at_ref, and set_python_version are used to locate edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51c034a7-6c96-45ec-8674-2b30a91f22cd
📒 Files selected for processing (1)
scripts/check_release_versions.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check_release_versions.sh`:
- Around line 283-288: The sed replacement in the non-TOML version uses a greedy
pattern "__version__ = \".*\"" which is inconsistent with other extractors and
the TOML setter; update the sed_inplace call that replaces "__version__ =
\"${new_version}\"" to use a non-greedy pattern "__version__ = \"[^\"]*\"" so it
matches only the version string (refer to the sed_inplace invocation in the
shown diff) and keep the subsequent grep check unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50ac6022-e99e-4ef6-bacd-1fb16ad2552f
📒 Files selected for processing (1)
scripts/check_release_versions.sh
| else | ||
| sed_inplace "s/__version__ = \".*\"/__version__ = \"${new_version}\"/" "$file" | ||
| if ! grep -q "__version__ = \"${new_version}\"" "$file"; then | ||
| echo -e " ${RED}FAILED to update $file${NC}" >&2 | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
Inconsistent regex: non-TOML setter still uses greedy .*.
The commit message states the fix uses non-greedy [^"]* regex, and the extraction functions (get_python_version, get_python_version_at_ref) and the TOML setter all use [^"]*. However, line 284 still uses .* which is greedy. For consistency and robustness, this should also use the non-greedy pattern.
🔧 Proposed fix
- sed_inplace "s/__version__ = \".*\"/__version__ = \"${new_version}\"/" "$file"
+ sed_inplace "s/__version__ = \"[^\"]*\"/__version__ = \"${new_version}\"/" "$file"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else | |
| sed_inplace "s/__version__ = \".*\"/__version__ = \"${new_version}\"/" "$file" | |
| if ! grep -q "__version__ = \"${new_version}\"" "$file"; then | |
| echo -e " ${RED}FAILED to update $file${NC}" >&2 | |
| return 1 | |
| fi | |
| else | |
| sed_inplace "s/__version__ = \"[^\"]*\"/__version__ = \"${new_version}\"/" "$file" | |
| if ! grep -q "__version__ = \"${new_version}\"" "$file"; then | |
| echo -e " ${RED}FAILED to update $file${NC}" >&2 | |
| return 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check_release_versions.sh` around lines 283 - 288, The sed
replacement in the non-TOML version uses a greedy pattern "__version__ = \".*\""
which is inconsistent with other extractors and the TOML setter; update the
sed_inplace call that replaces "__version__ = \"${new_version}\"" to use a
non-greedy pattern "__version__ = \"[^\"]*\"" so it matches only the version
string (refer to the sed_inplace invocation in the shown diff) and keep the
subsequent grep check unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check_release_versions.sh`:
- Around line 238-246: In get_python_version(), the non-TOML branch uses grep
without limiting to the first match; update the else branch so the grep for
'__version__' uses the same '-m1' flag as the TOML branch to ensure only the
first occurrence is returned (i.e., modify the grep call in the else branch that
extracts __version__ to include -m1).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab4065ec-2663-4611-ba93-4da43d4c66b3
📒 Files selected for processing (1)
scripts/check_release_versions.sh
…t.toml support The PYTHON_PACKAGES registry was missing smg-grpc-servicer. Also, the existing smg-grpc-proto entry pointed at __init__.py which uses importlib.metadata (extracting the package name, not the version). Switch both entries to read from pyproject.toml and update the get/set_python_version helpers to handle .toml files. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Use [^"]* instead of .* inside quoted matches for sed and awk version extraction/replacement. This prevents incorrect parsing if a version line ever contains multiple quoted strings. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Previously, Cargo.toml and Python version files were entirely excluded from the change count. This could hide real changes (e.g., dependency updates) in those files. Now only version-line changes are excluded — other modifications to manifest files are counted as code changes that require a version bump. Signed-off-by: Chang Su <chang.s.su@oracle.com>
The crate relocation (38ee0a5) moved crates from e.g. auth/ to crates/auth/. This caused check_release_versions.sh to see every file at the new path as "added", inflating diff counts and missing feat: commits at the old path (reporting patch instead of minor). Fix three places: - diff_count (crate + Python loops): include both old and new paths in git diff, use -M for rename detection, and filter out R100 (pure renames with no content changes) - detect_bump_level: search commits at both old and new paths so feat: commits from before the move are correctly detected - has_non_version_changes: use -M and both paths so pure renames of Cargo.toml/pyproject.toml don't count as content changes Result: smg-auth correctly drops off (0 real changes), tool-parser goes from 41 to 2 files, and several crates correctly report minor instead of patch (llm-tokenizer, kv-index, data-connector, etc). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
80df819 to
5933a0d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5933a0d557
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| echo "$content" | grep '__version__' | sed 's/.*"\(.*\)".*/\1/' | ||
| if [[ "$file" == *.toml ]]; then | ||
| echo "$content" | grep -m1 '^version' | sed 's/.*"\([^"]*\)".*/\1/' |
There was a problem hiding this comment.
When version_file is a TOML file, get_python_version_at_ref now parses the first ^version line directly from pyproject.toml; for older smg-grpc-proto revisions (e.g. before grpc_client/python/pyproject.toml switched to static version = "..."), that line is version = {attr = "smg_grpc_proto.__version__"}, so this returns smg_grpc_proto.__version__ instead of a semantic version. In release checks against those tags, current_version and tag_version will never match, so unchanged package versions are incorrectly treated as already bumped and required bumps can be skipped.
Useful? React with 👍 / 👎.
2 participants
Description
Problem
The
check_release_versions.shscript was missingsmg-grpc-servicerfrom itsPYTHON_PACKAGESregistry, so version bumps for the gRPC servicer were never checked during releases.Additionally, the existing
smg-grpc-protoentry pointed at__init__.pywhich usesimportlib.metadata.version()— the script'ssedextraction was pulling out the package name (smg-grpc-proto) instead of the actual version number, so bump detection never worked for it either.Solution
smg-grpc-servicertoPYTHON_PACKAGESpyproject.toml(the source of truth)get_python_version,get_python_version_at_ref, andset_python_versionto handle.tomlfiles alongside__init__.pyChanges
smg-grpc-protoversion file atpyproject.tomlinstead of__init__.pysmg-grpc-servicer|grpc_servicer|grpc_servicer/pyproject.tomlentry.tomlbranch to version get/set helpers using the samegrep -m1 '^version'pattern used for Cargo.tomlTest Plan
grep -m1 '^version' pyproject.toml | sed ...extracts0.4.2for both packages__init__.pycodepath is preserved for any future packages that use hardcoded__version__Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit