chore(ci): prune worktrees in xtest cleanup #338
Conversation
Summary of ChangesHello @dmihalcik-virtru, 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 refines the CI cleanup script by integrating more robust Git worktree management. The changes ensure that all Git worktree registrations are properly removed and any orphaned worktree entries are pruned, leading to a cleaner and more reliable CI environment. This prevents potential issues with lingering Git configurations after cleanup operations. 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 improves the CI cleanup script by correctly handling git worktrees, using worktree remove and worktree prune. This is a good enhancement to prevent stale worktree metadata. My review includes two suggestions for further improvement: one to address code duplication for maintainability by centralizing SDK path definitions, and another to refine the worktree removal logic to be more efficient and less verbose in the logs.
| get_bare_repo_path() { | ||
| local sdk=$1 | ||
| case "$sdk" in | ||
| js) | ||
| echo "$SCRIPT_DIR/../js/src/web-sdk.git" | ||
| ;; | ||
| java) | ||
| echo "$SCRIPT_DIR/../java/src/java-sdk.git" | ||
| ;; | ||
| go) | ||
| echo "$SCRIPT_DIR/../go/src/otdfctl.git" | ||
| ;; | ||
| *) | ||
| echo "" | ||
| ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
This get_bare_repo_path function duplicates logic from xtest/sdk/scripts/checkout-sdk-branch.sh for determining the bare repository path for each SDK. This duplication can lead to maintenance issues, as changes to SDK paths or names would need to be updated in both files.
To improve maintainability, consider consolidating this logic into a single, shared script or configuration file that both checkout-sdk-branch.sh and this script can use. For example, you could create a helper script that sets environment variables for the paths based on the SDK language.
| if ! git --git-dir="$bare_repo_path" worktree remove "$branch" --force; then | ||
| echo "Failed to remove worktree: $sdk#$branch" | ||
| fi | ||
| rm -rf "$branch" |
There was a problem hiding this comment.
The current logic for removing worktrees can be simplified and made less noisy. The git worktree remove command will fail and print an error if a directory is not a worktree, which creates unnecessary noise in the logs. Additionally, the rm -rf "$branch" command is redundant if git worktree remove succeeds, as it already removes the directory.
A cleaner approach is to attempt worktree remove and, only if it fails (for any reason, including not being a worktree), then fall back to rm -rf. This avoids the redundant call and suppresses unnecessary error messages for non-worktree directories.
| if ! git --git-dir="$bare_repo_path" worktree remove "$branch" --force; then | |
| echo "Failed to remove worktree: $sdk#$branch" | |
| fi | |
| rm -rf "$branch" | |
| if ! git --git-dir="$bare_repo_path" worktree remove "$branch" --force 2>/dev/null; then | |
| # If worktree remove fails (e.g., not a worktree), fall back to rm -rf. | |
| rm -rf "$branch" | |
| fi |
1 participant
No description provided.