feat(scripts): add PyPI proto version check to release version script - #510
Conversation
Extend check_release_versions.sh to verify that the smg-grpc-proto PyPI package version is bumped when proto files in grpc_client/proto/ change. What changed: - Phase 1b: detect proto file changes since the last tag, compare __version__ in grpc_client/python/smg_grpc_proto/__init__.py against the tagged version, and flag if unbumped - Phase 2: include the PyPI package in proposed fixes with bump level from conventional commits - Phase 3: auto-bump __version__ in __init__.py via sed when the user accepts the fix - Added get_pypi_version, get_pypi_version_at_ref, and set_pypi_version helpers following the same pattern as the Cargo crate helpers - Updated script header to document the new checks The proto source of truth is grpc_client/proto/; the Python package already symlinks to it (smg_grpc_proto/proto -> ../../proto), so no file sync logic is needed. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Summary of ChangesHello @slin1237, 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 enhances the release version checking script by adding robust validation for the Highlights
Changelog
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. 📝 WalkthroughWalkthroughThe release version check script is extended with Phase 1b to detect and manage Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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 extends the release version checking script to also handle the smg-grpc-proto PyPI package, which is a valuable addition. The implementation is largely correct and follows the existing style of the script. I've identified one critical syntax error that would prevent the script from running correctly and a medium-severity suggestion to refactor some duplicated logic for better long-term maintainability. After addressing these points, the changes should be solid.
| # Phase 2: Offer to fix | ||
| # --------------------------------------------------------------------------- | ||
| total_fixes=$(( ${#NEEDS_BUMP[@]} + ${#NEEDS_WS_SYNC[@]} )) | ||
| total_fixes=$(( ${#NEEDS_BUMP[@]} + ${#NEEDS_WS_SYNC[@]} + (${#PYPI_NEEDS_BUMP} > 0 ? 1 : 0) )) |
There was a problem hiding this comment.
This line introduces a syntax error. While bash's arithmetic context ((...)) supports a C-style ternary operator, the extra parentheses around the ternary expression (${#PYPI_NEEDS_BUMP} > 0 ? 1 : 0) are invalid and will cause the script to fail.
A more idiomatic and robust way to accomplish this in bash is to leverage the fact that a true condition evaluates to 1 in an arithmetic context. This is more concise and avoids the less common ternary syntax.
| total_fixes=$(( ${#NEEDS_BUMP[@]} + ${#NEEDS_WS_SYNC[@]} + (${#PYPI_NEEDS_BUMP} > 0 ? 1 : 0) )) | |
| total_fixes=$(( ${#NEEDS_BUMP[@]} + ${#NEEDS_WS_SYNC[@]} + (${#PYPI_NEEDS_BUMP} > 0) )) |
| if [[ -n "$PYPI_NEEDS_BUMP" ]]; then | ||
| IFS='|' read -r pypi_ver pypi_level <<< "$PYPI_NEEDS_BUMP" | ||
| pypi_new=$(bump_version "$pypi_ver" "$pypi_level") | ||
| if set_pypi_version "$PYPI_VERSION_FILE" "$pypi_new"; then | ||
| echo -e " ${GREEN}✓${NC} $PYPI_VERSION_FILE → v$pypi_new" | ||
| else | ||
| fix_failed=$((fix_failed + 1)) | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The logic to parse PYPI_NEEDS_BUMP and calculate the new version (lines 445-446) is a duplicate of the logic in Phase 2 (lines 393-394). This code duplication can lead to maintenance issues, as any changes to the format of PYPI_NEEDS_BUMP would need to be updated in two separate places.
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, you could calculate the new version once in Phase 2, store it in a variable, and reuse that variable here in Phase 3.
For example:
# Before Phase 2
pypi_new_version=""
# In Phase 2 (lines 392-396)
if [[ -n "$PYPI_NEEDS_BUMP" ]]; then
IFS='|' read -r pypi_ver pypi_level <<< "$PYPI_NEEDS_BUMP"
pypi_new_version=$(bump_version "$pypi_ver" "$pypi_level")
echo -e " $(bump_label "$pypi_level") smg-grpc-proto v$pypi_ver → v$pypi_new_version ($PYPI_VERSION_FILE)"
fi
# In Phase 3 (this block)
if [[ -n "$PYPI_NEEDS_BUMP" ]]; then
if set_pypi_version "$PYPI_VERSION_FILE" "$pypi_new_version"; then
echo -e " ${GREEN}✓${NC} $PYPI_VERSION_FILE → v$pypi_new_version"
else
fix_failed=$((fix_failed + 1))
fi
fi
1 participant
Summary
Extends
check_release_versions.shto also verify thesmg-grpc-protoPyPI package version is bumped when proto files change, closing the gap where proto changes could ship without a PyPI version bump.What changed
scripts/check_release_versions.shgrpc_client/proto/since the last tag, reads__version__fromgrpc_client/python/smg_grpc_proto/__init__.py, and flags if the version wasn't bumped__version__in__init__.pyviased_inplacewhen the user acceptsget_pypi_version,get_pypi_version_at_ref, andset_pypi_versionfollowing the same pattern as the existing Cargo helpersWhy
The script previously only checked Cargo crate versions. Proto file changes (which affect the PyPI-published
smg-grpc-protopackage) were not tracked, meaning proto changes could be released without a corresponding PyPI version bump.How
The PyPI version lives in
grpc_client/python/smg_grpc_proto/__init__.pyas__version__ = "X.Y.Z"and is read dynamically bypyproject.toml. The script diffsgrpc_client/proto/against the tag, compares current vs tagged__version__, and usesdetect_bump_level(conventional commits) to suggest the appropriate semver bump. The proto source of truth isgrpc_client/proto/; the Python package already symlinks to it (smg_grpc_proto/proto -> ../../proto).Test plan
bash scripts/check_release_versions.shagainst latest tagv1.0.1— correctly detected 3 proto files changed and proposedv0.3.3 → v0.4.0(minor)bash -nsyntax check passesSummary by CodeRabbit