tools: fix linting in update-protobuf.sh updater#347
tools: fix linting in update-protobuf.sh updater#347santigimeno wants to merge 1 commit intonode-v22.x-nsolid-v5.xfrom
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
santigimeno
force-pushed
the
node-v22.x-nsolid-v5.x
branch
from
580c207 to
b22a61c
Compare
santigimeno
force-pushed
the
santi/fix_lint_sh
branch
from
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>
2 participants
Summary by CodeRabbit