feat: improve batch block update #1303
Conversation
|
Summary of ChangesHello @vgpopov, 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 significantly enhances the batch block dependency update tooling by introducing a dedicated script for changeset generation, improving the robustness of repository cloning and dependency installation, and streamlining the pull request creation workflow. These changes aim to make the dependency update process more reliable, efficient, and user-friendly through better branch management, existing PR detection, and clearer script interactions. 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. 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
|
![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 significantly improves the batch dependency update scripts by adding documentation, modularizing the changeset creation logic, and making the PR creation process more robust. I've identified a few areas with bugs or opportunities for simplification to enhance maintainability and efficiency. Key issues include a bug in a log message, redundant code for reading branch names, and a logic error that incorrectly overwrites the PR tracking file. Overall, these are great improvements.
| if [[ -z "$BRANCH" ]]; then | ||
| DEFAULT_BRANCH="$BRANCH_FROM_FILE" | ||
| msg_info "Using existing branch '$BRANCH' from update-deps.txt" | ||
| fi |
There was a problem hiding this comment.
There's a bug in this log message. '$BRANCH' will be empty, leading to a confusing message like Using existing branch '' from update-deps.txt. Also, the if [[ -z "$BRANCH" ]] check is redundant. This logic can be simplified for clarity and correctness.
| if [[ -z "$BRANCH" ]]; then | |
| DEFAULT_BRANCH="$BRANCH_FROM_FILE" | |
| msg_info "Using existing branch '$BRANCH' from update-deps.txt" | |
| fi | |
| if [[ -n "$BRANCH_FROM_FILE" ]]; then | |
| DEFAULT_BRANCH="$BRANCH_FROM_FILE" | |
| msg_info "Using existing branch '$DEFAULT_BRANCH' from update-deps.txt as default." | |
| fi |
|
|
||
| # Summary | ||
| if [[ "${#CREATED_PRS[@]}" -gt 0 ]]; then | ||
| echo "${CREATED_PRS[@]}" | tee "$PRS_FILE" > /dev/null |
There was a problem hiding this comment.
This line overwrites the PRS_FILE with only the pull requests created in the current run, formatted as a single space-separated line. This defeats the purpose of the add_pr_to_file function, which is designed to append PRs and avoid duplicates across multiple runs. This line should be removed to ensure the tracking file works as intended.
| @@ -3,4 +3,34 @@ | |||
| For installation: | |||
| ```sh | |||
| curl | |||
There was a problem hiding this comment.
The curl command for installation is incomplete. This could be confusing for users trying to install the tool. Please provide the full command or remove the placeholder.
| # Create changeset file | ||
| { | ||
| echo '---' | ||
| while IFS= read -r name; do | ||
| if [ -n "$name" ] && [ "$name" != "undefined" ] && [ "$name" != "null" ]; then | ||
| printf '"%s": patch\n' "$name" | ||
| fi | ||
| done <<< "$ALL_NAMES" | ||
| echo '---' | ||
| echo '' | ||
| echo 'technical release' | ||
| } > "$CHANGESET_FILE" | ||
|
|
||
| # Verify last entry format is "...": patch and fix if necessary | ||
| LAST_ENTRY=$(awk 'flag{ if($0~/---/){exit}; if($0!=""){print} } $0~/---/{flag=1}' "$CHANGESET_FILE" | tail -n 1) | ||
|
|
||
| if [[ -z "$LAST_ENTRY" || ! "$LAST_ENTRY" =~ ^\".+\":\ patch$ ]]; then | ||
| msg_warn "Invalid last changeset entry: '$LAST_ENTRY'. Attempting to correct..." | ||
| GOOD_ENTRIES=$(awk 'flag{ if($0~/---/){exit}; if($0 ~ /^[\t ]*\".*\": patch[\t ]*$/){print} } $0~/---/{flag=1}' "$CHANGESET_FILE") | ||
|
|
||
| if [[ -z "$GOOD_ENTRIES" ]]; then | ||
| msg_warn "No valid changeset entries found; removing $CHANGESET_FILE" | ||
| rm -f "$CHANGESET_FILE" | ||
| return 1 | ||
| else | ||
| { | ||
| echo '---' | ||
| printf "%s\n" "$GOOD_ENTRIES" | ||
| echo '---' | ||
| echo '' | ||
| echo 'technical release' | ||
| } > "$CHANGESET_FILE" | ||
| msg_success "Changeset corrected in $CHANGESET_FILE" | ||
| fi | ||
| else | ||
| msg_success "Changeset created: $CHANGESET_FILE" | ||
| fi |
There was a problem hiding this comment.
This block of code for creating and then verifying the changeset file is overly complex and hard to maintain. The use of awk and tail to validate the file content after writing it is fragile. This can be simplified by building the changeset frontmatter in a variable first, which is more readable and less error-prone.
# Create changeset file
declare -a frontmatter_lines=()
while IFS= read -r name; do
if [[ -n "$name" ]]; then
frontmatter_lines+=("$(printf '"%s": patch' "$name")")
fi
done <<< "$ALL_NAMES"
if [ ${#frontmatter_lines[@]} -eq 0 ]; then
msg_warn "No packages found for changeset."
return 0
fi
{
echo "---"
printf '%s\n' "${frontmatter_lines[@]}"
echo "---"
echo ""
echo "technical release"
} > "$CHANGESET_FILE"
msg_success "Changeset created: $CHANGESET_FILE"| if [[ -f "$UPDATE_DEPS_FILE" ]]; then | ||
| if grep -q "^branch=" "$UPDATE_DEPS_FILE"; then | ||
| BRANCH=$(grep "^branch=" "$UPDATE_DEPS_FILE" | cut -d'=' -f2-) | ||
| msg_info "Using branch from update-deps.txt: $BRANCH" | ||
| else | ||
| msg_error "--branch is required (or create update-deps.txt with branch=<name> when on main)" | ||
| exit 1 | ||
| fi | ||
| else | ||
| msg_error "--branch is required (or create update-deps.txt with branch=<name> when on main)" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This block for reading the branch from update-deps.txt is duplicated in several places (e.g., finish command, update-deps.sh). It can also be simplified to be more concise and avoid redundant else blocks. Consider extracting this logic into a helper function to improve maintainability.
| if [[ -f "$UPDATE_DEPS_FILE" ]]; then | |
| if grep -q "^branch=" "$UPDATE_DEPS_FILE"; then | |
| BRANCH=$(grep "^branch=" "$UPDATE_DEPS_FILE" | cut -d'=' -f2-) | |
| msg_info "Using branch from update-deps.txt: $BRANCH" | |
| else | |
| msg_error "--branch is required (or create update-deps.txt with branch=<name> when on main)" | |
| exit 1 | |
| fi | |
| else | |
| msg_error "--branch is required (or create update-deps.txt with branch=<name> when on main)" | |
| exit 1 | |
| fi | |
| if [[ -f "$UPDATE_DEPS_FILE" ]] && grep -q "^branch=" "$UPDATE_DEPS_FILE"; then | |
| BRANCH=$(grep "^branch=" "$UPDATE_DEPS_FILE" | cut -d'=' -f2-) | |
| msg_info "Using branch from update-deps.txt: $BRANCH" | |
| else | |
| msg_error "--branch is required (or create update-deps.txt with branch=<name> when on main)" | |
| exit 1 | |
| fi |
| msg_info "Running pnpm install to ensure dependencies are up to date..." | ||
| pnpm install || msg_warn "pnpm install failed in $DIR" |
There was a problem hiding this comment.
The pnpm install command here is redundant. An installation is already performed on line 260 if dependency files have changed, which is the primary case for needing to run it. The clone-milab-open.sh script also ensures dependencies are installed initially. Removing this unconditional install will improve script efficiency.
| @@ -3,4 +3,34 @@ | |||
| For installation: | |||
| ```sh | |||
| curl | |||
|
|
||
| (cd "$target_dir" && pnpm i) || msg_error "Could not install dependencies" | ||
| if [ -d "$target_dir" ]; then | ||
| if [ -f "$target_dir/package.json" ]; then |
There was a problem hiding this comment.
Let's also check pnpm-workspace file. platforma-open may keep also repos with single package within.
No description provided.