-
Notifications
You must be signed in to change notification settings - Fork 0
fix(snaplet): add snaplet target DB var to SNAPSHOT_PATHs #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis change modifies the snapshot path construction in several tasks within Possibly related PRs
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/snaplet/Taskfile.yaml (3)
121-121
: Update download command to use new path
Theaws s3 cp
command now correctly references the DB-specific path. For safety, consider quoting both the source URI and destination path ("{{...}}"
) to guard against special characters or spaces.
133-133
: RequireSNAPLET_TARGET_DB_NAME
for restores
Good addition of the DB name requirement. Optionally, you could alsorequire
SNAPSHOT_ENV
here to ensure the environment is explicit and avoid default fallbacks.
145-145
: Include database identifier in restore:noprompt path
Consistent override for non-prompt restores. Similar to download, consider quotingSNAPSHOT_PATH
to align with the quoted declaration in the download task.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/snaplet/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/snaplet/Taskfile.yaml
🔇 Additional comments (2)
lib/snaplet/Taskfile.yaml (2)
114-114
: Include database identifier in download snapshot path
Great update to scope downloads per target DB by overridingSNAPSHOT_PATH
with{{.SNAPLET_TARGET_DB_NAME}}
.
130-130
: Include database identifier in restore path
OverridingSNAPSHOT_PATH
to include the DB name correctly isolates restores per database.
- printf "\n\n⬇ Snapshot downloaded to {{.SNAPSHOT_PATH}}\n\n" | ||
|
||
restore: | ||
desc: Restores the given RESTORE_SNAPSHOT to the SNAPLET_TARGET_DATABASE_URL database. | ||
silent: true | ||
env: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SNAPLET_TARGET_DATABASE_URL is not used in the actual command. Snaplet reads this from the environment, so if we are passing this in as a Taskfile variable input argument, it has to be set as environment variable.
🤖 I have created a release *beep* *boop* --- ## [0.4.0](v0.3.1...v0.4.0) (2025-06-04) ### Features * add commands to download cursor rules ([#20](#20)) ([de411c2](de411c2)) * small solutions for running task os:sync-all ([#23](#23)) ([0b2ddc3](0b2ddc3)) ### Bug Fixes * **snaplet:** add snaplet target DB var to SNAPSHOT_PATHs ([#25](#25)) ([f5e70c6](f5e70c6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Follow up to a previous PR #13 where support for
SNAPLET_TARGET_DB_NAME
was added, where there are use cases when using the same Taskfile commands, but for different DB connections and DB names.The
SNAPSHOT_PATH
where the snapshots are stored, did not have theSNAPLET_TARGET_DB_NAME
identifier. This PR adds that.This is needed because if storing multiple DB snapshots, all snapshots are stored in the same directory of the .SNAPSHOT_ENV
Summary by CodeRabbit