feat: add configurable backup file prefix - #356
Conversation
📝 WalkthroughWalkthroughAdds optional ChangesBackup prefix configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Hi @Antoninj , Could you describe a bit more you use case ? Tests are in : https://github.com/Portabase/e2e-tests you can are test to it directly This e2e tests are called in PR, like a component |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/channel/components/storages/s3.ts`:
- Around line 34-39: Add unit tests for buildKey to cover the centralized S3 key
normalization logic used by upload/get/delete/ping/copy. Focus on edge cases
around S3Config.prefix and path handling: empty or whitespace-only prefix,
prefix containing only slashes, path values with leading/trailing slashes, and
paths with multiple internal slashes. Use buildKey and S3Config as the main
symbols to locate the behavior and verify the returned key strings for each
case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fbc8effb-dc47-40c1-bf7e-80df479d9bde
📒 Files selected for processing (3)
src/features/channel/components/storages/s3.form.tsxsrc/features/channel/components/storages/s3.schema.tssrc/features/channel/components/storages/s3.ts
📜 Review details
🔇 Additional comments (3)
src/features/channel/components/storages/s3.schema.ts (1)
9-9: LGTM!src/features/channel/components/storages/s3.form.tsx (1)
87-99: LGTM!src/features/channel/components/storages/s3.ts (1)
34-39: 🗄️ Data Integrity & IntegrationNo backward-compatibility issue.
prefixis opt-in; existing S3 configs keep using the raw object path, so there’s no key-namespace migration here.> Likely an incorrect or invalid review comment.
|
-> Could you describe a bit more you use case ? The use case is the following: I use the same S3 bucket for multiple backups (not only portabase ones). Currently, portabase hardcodes the prefix to
thanks I'll take a look and see if adding an E2E test makes sense for my changes. But in any case, I'd need to get those changes in first |
|
Added E2E coverage in Portabase/e2e-tests#11. It fills the new S3 Prefix field for the existing S3/R2 storage channel flow and verifies the value persists when reopening the channel. I left it as a draft because it depends on this PR introducing the Prefix field. |
|
Hi @Antoninj, I now understand your use case. This PR will not work as-is because uploads are handled partly on the dashboard side at some point, but mainly by the agent when dispatching backup files to storage after the backup is completed. What I suggest is adding an environment variable on the dashboard side, for example I think this would be more generic and would apply consistently across the whole ecosystem. Could you open an issue so we can continue the discussion there? I can work on it ASAP, but if you want, you can also start implementing the dashboard-side part, and I can finish the agent-side integration. Regards, |
|
Sounds good I'll do that thanks |
Feel free to ask if you have any questions. I’d be happy to help. |
|
Thanks. I won't have time to iterate on this before next week end though. |
No problem, ping me when you have time |
8646ec9 to
4a77932
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/storages/utils/storages.helpers.ts`:
- Around line 23-25: Update getBackupFilePrefix and the dashboard-to-agent
configuration flow so the same normalized BACKUP_FILE_PREFIX is propagated to
agent-managed backups. Ensure agent uploads and subsequent storage operations
use this prefix instead of defaulting to /backups, while preserving the existing
"backups" fallback when the variable is unset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10e8fbf6-8f74-4174-8bd3-4bf3b08c047f
📒 Files selected for processing (3)
.env.examplesrc/env.mjssrc/features/storages/utils/storages.helpers.ts
📜 Review details
🔇 Additional comments (2)
.env.example (1)
76-79: LGTM!src/env.mjs (1)
69-70: LGTM!Also applies to: 128-131, 159-159, 215-215
| function getBackupFilePrefix(): string { | ||
| return env.BACKUP_FILE_PREFIX?.trim().replace(/^\/+|\/+$/g, "") || "backups"; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Propagate the prefix to agent-managed backups.
This helper only changes paths created by the dashboard. Agent uploads will continue using their existing /backups prefix unless BACKUP_FILE_PREFIX is included in the dashboard-to-agent configuration contract. That leaves backup sets split across prefixes and can make subsequent storage operations target inconsistent keys. Pass the normalized prefix to the agent, or centralize path construction before applying this dashboard-only change.
Also applies to: 63-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/storages/utils/storages.helpers.ts` around lines 23 - 25, Update
getBackupFilePrefix and the dashboard-to-agent configuration flow so the same
normalized BACKUP_FILE_PREFIX is propagated to agent-managed backups. Ensure
agent uploads and subsequent storage operations use this prefix instead of
defaulting to /backups, while preserving the existing "backups" fallback when
the variable is unset.
I updated the implementation based on your recommendations and linked the PR to an issue in the repo. |
Summary
Adds an optional dashboard environment variable,
BACKUP_FILE_PREFIX, for the object-key namespace used when backup files are sent to configured storage channels.backups, preserving existing deployments.backupsfor an empty value..env.example.Coordinated changes
Deploy this PR together with the agent change before setting a custom prefix.
Validation
git diff --checkpnpmis unavailable in the development environment.Summary by CodeRabbit
backupslocation.