fix: version sync workspace - #615
Conversation
🦋 Changeset detectedLatest commit: b4654d0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 a critical issue in the version synchronization process, ensuring that the Highlights
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. Footnotes
|
|
/gemini review |
90252aa to
7b541f8
Compare
There was a problem hiding this comment.
Code Review
This pull request fixes the version-sync.sh script to correctly update the Cargo.toml file for the google-workspace-cli crate, bumping its version to 0.21.0. The review highlights a potential race condition in the script due to the use of a hardcoded temporary file name, suggesting mktemp for a more robust solution.
|
/gemini review |
94a6e08 to
b4654d0
Compare
|
/gemini review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #615 +/- ##
=======================================
Coverage 71.06% 71.06%
=======================================
Files 44 44
Lines 20334 20334
=======================================
Hits 14450 14450
Misses 5884 5884 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with the version-sync.sh script, updating it to correctly target the Cargo.toml file for the google-workspace-cli crate. This change ensures that the CLI crate and all associated skill metadata versions are consistently bumped from 0.20.1 to 0.21.0. A review comment suggests improving the robustness of the awk script's version pattern matching to account for variable whitespace, preventing potential failures during the versioning process.
| tmp=$(mktemp) | ||
| awk -v ver="$VERSION" ' | ||
| /^\[package\]/ { in_pkg=1 } | ||
| /^\[/ && !/^\[package\]/ { in_pkg=0 } |
There was a problem hiding this comment.
The current awk script is brittle as it assumes a specific formatting for the version key in Cargo.toml (version = ...). TOML allows for different spacing, such as version=... or version = .... The script will fail to update the version if the spacing is different, which could lead to an inconsistent state in the repository during the release process. It's better to make the pattern matching more flexible to handle optional whitespace.
| /^\[/ && !/^\[package\]/ { in_pkg=0 } | |
| in_pkg && /^version[[:space:]]*=/ { $0 = "version = \"" ver "\"" } |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
Code Review
This pull request updates the version-sync.sh script to correctly synchronize versions across all workspace Cargo.toml files (for google-workspace and google-workspace-cli) and Cargo.lock, aligning them with the version in package.json. The script now iterates through crates/*/Cargo.toml and uses mktemp for temporary file operations. A review comment suggests adding a trap for proper cleanup of temporary files created by mktemp in scripts/version-sync.sh to prevent potential clutter and disk space issues.
| for cargo_toml in crates/*/Cargo.toml; do | ||
| tmp=$(mktemp) | ||
| awk -v ver="$VERSION" ' | ||
| /^\[package\]/ { in_pkg=1 } | ||
| /^\[/ && !/^\[package\]/ { in_pkg=0 } | ||
| in_pkg && /^version = / { $0 = "version = \"" ver "\"" } | ||
| { print } | ||
| ' "$cargo_toml" > "$tmp" && mv "$tmp" "$cargo_toml" | ||
| done |
There was a problem hiding this comment.
The use of mktemp without proper cleanup can lead to temporary files being left behind, especially if the script exits unexpectedly (e.g., if the awk command fails). This can clutter the temporary directory and potentially fill up disk space over time in a CI environment.
To ensure temporary files are always cleaned up, it's best practice to wrap the file modification logic in a subshell with a trap for the EXIT signal. This guarantees that the temporary file is removed when the subshell exits, regardless of whether it's due to success or failure.
| for cargo_toml in crates/*/Cargo.toml; do | |
| tmp=$(mktemp) | |
| awk -v ver="$VERSION" ' | |
| /^\[package\]/ { in_pkg=1 } | |
| /^\[/ && !/^\[package\]/ { in_pkg=0 } | |
| in_pkg && /^version = / { $0 = "version = \"" ver "\"" } | |
| { print } | |
| ' "$cargo_toml" > "$tmp" && mv "$tmp" "$cargo_toml" | |
| done | |
| for cargo_toml in crates/*/Cargo.toml; do | |
| ( # Run in a subshell to scope the trap and ensure cleanup | |
| set -e # It's good practice to be explicit in subshells | |
| tmp=$(mktemp) | |
| trap "rm -f '$tmp'" EXIT | |
| awk -v ver="$VERSION" ' | |
| /^\[package\]/ { in_pkg=1 } | |
| /^\[/ && !/^\[package\]/ { in_pkg=0 } | |
| in_pkg && /^version = / { $0 = "version = \"" ver "\"" } | |
| { print } | |
| ' "$cargo_toml" > "$tmp" && mv "$tmp" "$cargo_toml" | |
| ) | |
| done |
No description provided.