tools: add script to regenerate protofiles #326
Conversation
WalkthroughA new shell script named Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant regen_script as regenrate_proto_files.sh
participant utils as utils.sh
participant ProtoGen as Protobuf Regeneration Functions
User->>regen_script: Execute script
regen_script->>regen_script: Set error/debug flags, determine directories
regen_script->>utils: Source utility functions
regen_script->>regen_script: Create temp workspace & register cleanup
regen_script->>ProtoGen: regenerate_proto_otel
regen_script->>ProtoGen: regenerate_proto_agents
regen_script->>regen_script: Cleanup temp workspace (on exit/interruption)
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 (
|
| [ -x "$NODE" ] || NODE=$(command -v node) | ||
|
|
||
| # shellcheck disable=SC1091 | ||
| . "$BASE_DIR/tools/dep_updaters/utils.sh" |
There was a problem hiding this comment.
Consider adding a check to ensure that '$BASE_DIR/tools/dep_updaters/utils.sh' exists before sourcing it, to provide a clearer error message if the file is missing.
| cleanup () { | ||
| EXIT_CODE=$? | ||
| [ -d "$WORKSPACE" ] && rm -rf "$WORKSPACE" | ||
| exit $EXIT_CODE |
There was a problem hiding this comment.
The 'cleanup' function uses 'exit', which will always terminate the shell. If this script is ever sourced, this could cause unexpected termination. If the script is only ever executed directly, this is fine, but if there's a chance it could be sourced, consider using 'return' instead.
| @@ -0,0 +1,27 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
The filename contains a typo: 'regenarate' should be 'regenerate'. Consider renaming the script to 'regenerate_proto_files.sh' for clarity and consistency.
There was a problem hiding this comment.
Pull Request Overview
Adds a new shell script to automate regeneration of protobuf files when updating dependencies.
- Introduces
tools/regenarate_proto_files.shto invoke OpenTelemetry and agent proto regen functions. - Sets up workspace, locates
nodeexecutable, and sources shared utilities.
Comments suppressed due to low confidence (2)
tools/regenarate_proto_files.sh:3
- [nitpick] Add a usage/help section (e.g.,
--helpflag) at the top to explain how to run the script and any required environment variables.
# Shell script to update protobuf in the source tree to specific version
tools/regenarate_proto_files.sh:14
- There are no tests or verifications for this new script. Consider adding a CI check or basic smoke test to confirm the regeneration works as expected.
WORKSPACE=$(mktemp -d 2> /dev/null || mktemp -d -t 'tmp')
| @@ -0,0 +1,27 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
The script filename and any references to 'regenarate' are misspelled; it should be 'regenerate'. Consider renaming the file and updating all occurrences.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tools/regenarate_proto_files.sh (2)
2-2: Enhance error handling and safety in shell optionsConsider extending
set -extoset -euo pipefail -x(-ufor unset variables,-o pipefailfor pipeline failures) to catch more error conditions early.
14-14: Use a descriptive temp directory templateThe fallback
mktemp -d -t 'tmp'may be too generic. Prefer a unique template, e.g.:-WORKSPACE=$(mktemp -d 2> /dev/null || mktemp -d -t 'tmp') +WORKSPACE=$(mktemp -d 2> /dev/null || mktemp -d -t 'regen_proto.XXXXXX')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/regenarate_proto_files.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-tarball
- GitHub Check: test-macOS
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: lint-js-and-md
- GitHub Check: build-docs
🔇 Additional comments (2)
tools/regenarate_proto_files.sh (2)
11-13: Verifyutils.shexists before sourcingSourcing a missing or unreadable
utils.shwill error without context. Add a pre-check:if [ ! -r "$BASE_DIR/tools/dep_updaters/utils.sh" ]; then echo "Error: cannot read utils.sh at $BASE_DIR/tools/dep_updaters/utils.sh" >&2 exit 1 fiEnsure the relative path to
utils.shis correct.
22-27: Verify regenerate function signatures
regenerate_proto_otelis called with three arguments ($WORKSPACE,$DEPS_DIR,$BASE_DIR), whereasregenerate_proto_agentsuses only two ($BASE_DIR,$DEPS_DIR). Confirm these match their definitions inutils.sh—the order or count may need adjustment.
| #!/bin/sh | ||
| set -ex | ||
| # Shell script to update protobuf in the source tree to specific version |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct filename typo and add project header
- The script is named
regenarate_proto_files.sh; it should beregenerate_proto_files.shto fix the spelling. - Insert the standard NodeSource/Node.js license and contributor header above the shebang for compliance with project guidelines.
🤖 Prompt for AI Agents
In tools/regenarate_proto_files.sh at lines 1 to 3, rename the file to
regenerate_proto_files.sh to correct the spelling mistake. Additionally, add the
standard NodeSource/Node.js license and contributor header above the existing
shebang line to comply with project guidelines.
| [ -z "$NODE" ] && NODE="$BASE_DIR/out/Release/node" | ||
| [ -x "$NODE" ] || NODE=$(command -v node) | ||
|
|
There was a problem hiding this comment.
Fail early if Node.js executable is not found
If neither $BASE_DIR/out/Release/node nor node in PATH is executable, $NODE will be empty, causing later commands to fail obscurely. Add a check:
if [ -z "$NODE" ] || [ ! -x "$NODE" ]; then
echo "Error: Node.js executable not found." >&2
exit 1
fi🤖 Prompt for AI Agents
In tools/regenarate_proto_files.sh around lines 8 to 10, the script sets the
NODE variable to a Node.js executable path but does not verify if it is valid or
executable. Add a check after setting NODE to test if NODE is empty or not
executable, and if so, print an error message "Error: Node.js executable not
found." to stderr and exit the script with status 1 to fail early and avoid
obscure errors later.
| cleanup () { | ||
| EXIT_CODE=$? | ||
| [ -d "$WORKSPACE" ] && rm -rf "$WORKSPACE" | ||
| exit $EXIT_CODE | ||
| } | ||
| trap cleanup INT TERM EXIT |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent recursive trap invocation in cleanup
Calling exit inside the cleanup trap re-triggers the EXIT trap. Disable traps at the start:
cleanup () {
+ trap - INT TERM EXIT
EXIT_CODE=$?
[ -d "$WORKSPACE" ] && rm -rf "$WORKSPACE"
exit $EXIT_CODE
}🤖 Prompt for AI Agents
In tools/regenarate_proto_files.sh around lines 15 to 20, the cleanup function
calls exit which retriggers the EXIT trap causing recursive invocation. To fix
this, disable all traps at the start of the cleanup function by adding a command
to unset or ignore traps before calling exit, preventing the trap from being
called recursively.
PR-URL: #326 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
|
Landed in 2492fc8 |
PR-URL: #326 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Summary by CodeRabbit