feat: small solutions for running task os:sync-all #23
Conversation
westonplatter
changed the title
WalkthroughThe changes update the Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/os-modules/Taskfile.yaml (1)
77-81: Improve clone step portability and error handling.
echo -eisn’t standardized across shells—preferprintf.- Add
set -euo pipefail(or|| exit 1) so a failedgit clonestops the process instead of causing later errors whencding into a missing directory.Example diff:
- echo -e "\n\n🚀 Processing ---------------- $module \n" + printf "\n\n🚀 Processing ---------------- %s\n" "$module" if [ ! -d ../$module ]; then - echo "🧲 Cloning repository..." + printf "🧲 Cloning repository: %s\n" "$module" - git clone "[email protected]:masterpointio/$module.git" ../$module + git clone "[email protected]:masterpointio/$module.git" ../"$module" || exit 1 fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/os-modules/Taskfile.yaml(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.yaml`: You are well-versed in writing and reviewing YAML configurations for the Task tool (https://taskfile.dev/). Provide recommendations for clarity, maintainability, and a...
**/*.yaml: You are well-versed in writing and reviewing YAML configurations for the Task tool (https://taskfile.dev/).
Provide recommendations for clarity, maintainability, and adherence to Taskfile best practices, including usage of variables, environment blocks, and includes.
These configurations may also contain embedded Bash scripts or commands.
Demonstrate bash scripting best practices such as error handling, secure variable expansions, and clear documentation.
lib/os-modules/Taskfile.yaml
🔇 Additional comments (4)
lib/os-modules/Taskfile.yaml (4)
11-11: Module list update is correct.
Replacingterraform-postgres-automationwithterraform-postgres-config-dbs-users-rolesinDEFAULT_MODULESaligns with the new repository name and looks good.
84-84: Skip: Informational pull log.
This echo is purely informational and doesn’t require changes.
88-88: Skip: Branch creation announcement.
The log helps with clarity; no action needed here.
163-163: Verify DELETE_EXISTING_SYNC_BRANCH propagation insync-all.
You’ve addedDELETE_EXISTING_SYNC_BRANCHundersync-all, but Task’s variable scoping may not automatically pass it to thepull-and-branchsubtask. Please confirm that this flag is honored downstream or explicitly pass it when invokingtask: pull-and-branch.
| if git branch --list "{{.SYNC_BRANCH}}" | grep -q "{{.SYNC_BRANCH}}" && [ "{{.DELETE_EXISTING_SYNC_BRANCH}}" = "false" ]; then | ||
| echo "⏭️ Branch {{.SYNC_BRANCH}} already exists, skipping creation." | ||
|
|
||
| # If branch exists and delete option is turned on - delete and create new branch | ||
| elif git branch --list "{{.SYNC_BRANCH}}" | grep -q "{{.SYNC_BRANCH}}" && [ "{{.DELETE_EXISTING_SYNC_BRANCH}}" = "true" ]; then | ||
| echo "⏭️ Branch {{.SYNC_BRANCH}} already exists, deleting it." | ||
| git branch -D {{.SYNC_BRANCH}} | ||
| git checkout -b {{.SYNC_BRANCH}} | ||
|
|
||
| # If branch does not exist - create new branch | ||
| else | ||
| git checkout -b {{.SYNC_BRANCH}} | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a more robust branch-existence check.
Relying on git branch --list | grep can match substrings or aliases. Instead, use:
git rev-parse --verify --quiet "refs/heads/{{.SYNC_BRANCH}}"to detect exactly whether the branch exists. For example:
- if git branch --list "{{.SYNC_BRANCH}}" | grep -q "{{.SYNC_BRANCH}}"
+ if git rev-parse --verify --quiet "refs/heads/{{.SYNC_BRANCH}}"; thenAlso consider handling remote branches (prune or delete them if needed) and ensuring any branch deletion errors abort the task.
🤖 Prompt for AI Agents
In lib/os-modules/Taskfile.yaml around lines 91 to 103, the current branch
existence check using 'git branch --list | grep' can produce false positives by
matching substrings. Replace this with 'git rev-parse --verify --quiet
"refs/heads/{{.SYNC_BRANCH}}"' to precisely check if the branch exists.
Additionally, enhance the script to handle remote branches by pruning or
deleting them if necessary, and ensure that any errors during branch deletion
cause the task to abort to prevent inconsistent states.
| DELETE_EXISTING_SYNC_BRANCH: "{{.DELETE_EXISTING_SYNC_BRANCH | default false}}" | ||
| cmds: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Promote DELETE_EXISTING_SYNC_BRANCH to global vars.
You’ve duplicated DELETE_EXISTING_SYNC_BRANCH in both pull-and-branch and sync-all. Extracting it into the top-level vars: block will DRY up this Taskfile and guarantee it’s available to all tasks and subtasks without redefinition.
🤖 Prompt for AI Agents
In lib/os-modules/Taskfile.yaml around lines 70 to 71, the variable
DELETE_EXISTING_SYNC_BRANCH is duplicated in both pull-and-branch and sync-all
tasks. To fix this, move the DELETE_EXISTING_SYNC_BRANCH definition from these
individual tasks into the top-level vars: block of the Taskfile. This will make
the variable globally available to all tasks and subtasks, eliminating
duplication and improving maintainability.
what
DEFAULT_MODULESrepos. I added a graceful response where we git clone the repos for the user.DELETE_EXISTING_SYNC_BRANCH, defaulting tofalse(to keep existing workflow) so the process conditionally deletes theSYNC_BRANCHand creates a newSYNC_BRANCH.Summary by CodeRabbit