-
Notifications
You must be signed in to change notification settings - Fork 10
tools: fix linting in update-protobuf.sh updater #347
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
Conversation
WalkthroughA shell script for updating protobuf dependencies was modified to remove an unused variable assignment. The tarball download method remains as a commented-out option, while the script continues to use Changes
Estimated code review effort1 (~2 minutes) Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tools/dep_updaters/update-protobuf.sh (2)
45-47: Dead-code comment keeps an unused variable around – consider pruning for clarityThe now-commented
PROTOBUF_TARBALLassignment keeps the very variable that caused the original lint error (SC2034) in the file. Unless you expect downstream users to uncomment this block frequently, remove the assignment entirely or move the variable inside the tarball branch to avoid future confusion.-# PROTOBUF_TARBALL="protobuf-$NEW_VERSION.tar.gz" +# # Only needed if using the tarball flow below +# # PROTOBUF_TARBALL="protobuf-$NEW_VERSION.tar.gz"
63-69: Surface HTTP errors and makecurlsafer
curl -sLhides both progress and HTTP error codes. Add--failso the script exits on 4xx/5xx and keep-Sto still suppress progress while printing errors.-curl -sL -o "$ABSEIL_TARBALL" "https://github.com/abseil/abseil-cpp/archive/refs/tags/$ABSEIL_TARBALL" +curl -sSLf -o "$ABSEIL_TARBALL" \ + "https://github.com/abseil/abseil-cpp/archive/refs/tags/$ABSEIL_TARBALL"This prevents silent corruption of the workspace when the download fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/dep_updaters/update-protobuf.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-docs
- GitHub Check: lint-js-and-md
- GitHub Check: test-macOS
- GitHub Check: build-tarball
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
🔇 Additional comments (1)
tools/dep_updaters/update-protobuf.sh (1)
38-38: Parameter expansion silently drops a5.prefix – verify this is intentional
PROTOBUF_REF="v${NEW_VERSION#5.}"strips a leading5.from the version passed by the user.
This means5.25.0→v25.0, but26.1→v26.1. If the intent is simplyv$NEW_VERSION, use straightforward concatenation to avoid surprising behaviour.-PROTOBUF_REF="v${NEW_VERSION#5.}" +PROTOBUF_REF="v$NEW_VERSION"Please confirm the business logic before adjusting.
580c207 to
b22a61c
Compare
59bfaec to
21e73cb
Compare
PR-URL: #347 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
|
Landed in ce20cc6 |
PR-URL: #347 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Summary by CodeRabbit