feat: add AWS S3 sync and management UI config backup - #673
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Summary of ChangesHello @shunkakinoki, 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 configuration management for the 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
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR updates NixOS configuration file sources for clipproxyapi templates, normalizes model version strings from Changes
Sequence DiagramsequenceDiagram
participant startup as Startup Script
participant local as Local Filesystem
participant objectstore as Objectstore (S3/R2)
participant env as Environment Variables
startup->>local: Create objectstore config dir
startup->>local: Create backup config dir
startup->>local: Copy config.yaml to both dirs
startup->>env: Check for objectstore credentials<br/>and endpoint
alt Credentials Present
startup->>objectstore: s3 sync objectstore config dir
startup->>objectstore: s3 sync backup config dir
else No Credentials
startup->>startup: Skip remote sync
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
✨ 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 |
Mesa DescriptionTL;DRAdded AWS S3 sync and management UI config backup. What changed?
Description generated by Mesa. Update settings |
![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 introduces AWS S3 synchronization for configuration files, including a backup mechanism. The changes look good overall, especially the configuration templating updates. However, I've identified a couple of areas for improvement in the start.sh script. Specifically, there's a high-risk issue with silent error handling during the S3 sync process that could lead to data inconsistencies. I've also suggested a refactoring to reduce code duplication and improve maintainability, and pointed out a minor redundancy.
| if [ -n "$OBJECTSTORE_ENDPOINT" ] && [ -n "$OBJECTSTORE_ACCESS_KEY" ] && [ -n "$OBJECTSTORE_SECRET_KEY" ]; then | ||
| AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$OBJECTSTORE_ENDPOINT" \ | ||
| --no-progress \ | ||
| "$OBJECTSTORE_CONFIG_DIR/" \ | ||
| "s3://${OBJECTSTORE_BUCKET}/config/" || true | ||
|
|
||
| AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$OBJECTSTORE_ENDPOINT" \ | ||
| --no-progress \ | ||
| "$BACKUP_CONFIG_DIR/" \ | ||
| "s3://${OBJECTSTORE_BUCKET}/backup/config/" || true | ||
| fi |
There was a problem hiding this comment.
The two aws s3 sync blocks are nearly identical and they suppress errors using || true. This is risky because any failure to sync the configuration to S3 will be silently ignored, which could lead to stale configurations on the object store and potential data loss if an old config is pulled later.
I recommend refactoring this logic into a function. This would not only reduce code duplication but also allow for proper error handling, such as logging a warning message on failure. This would make the script more robust and easier to maintain.
| if [ -n "$OBJECTSTORE_ENDPOINT" ] && [ -n "$OBJECTSTORE_ACCESS_KEY" ] && [ -n "$OBJECTSTORE_SECRET_KEY" ]; then | |
| AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$OBJECTSTORE_ENDPOINT" \ | |
| --no-progress \ | |
| "$OBJECTSTORE_CONFIG_DIR/" \ | |
| "s3://${OBJECTSTORE_BUCKET}/config/" || true | |
| AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$OBJECTSTORE_ENDPOINT" \ | |
| --no-progress \ | |
| "$BACKUP_CONFIG_DIR/" \ | |
| "s3://${OBJECTSTORE_BUCKET}/backup/config/" || true | |
| fi | |
| if [ -n "$OBJECTSTORE_ENDPOINT" ] && [ -n "$OBJECTSTORE_ACCESS_KEY" ] && [ -n "$OBJECTSTORE_SECRET_KEY" ]; then | |
| sync_to_s3() { | |
| local source_dir="$1" | |
| local dest_path="$2" | |
| if ! AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$OBJECTSTORE_ENDPOINT" \ | |
| --no-progress \ | |
| "${source_dir}/" \ | |
| "s3://${OBJECTSTORE_BUCKET}/${dest_path}/"; then | |
| echo "⚠️ Failed to sync ${source_dir} to S3. Continuing..." >&2 | |
| fi | |
| } | |
| sync_to_s3 "$OBJECTSTORE_CONFIG_DIR" "config" | |
| sync_to_s3 "$BACKUP_CONFIG_DIR" "backup/config" | |
| fi |
| BACKUP_CONFIG_DIR="$CONFIG_DIR/backup/config" | ||
| BACKUP_CONFIG="$BACKUP_CONFIG_DIR/config.yaml" | ||
| mkdir -p "$OBJECTSTORE_CONFIG_DIR" "$BACKUP_CONFIG_DIR" | ||
| rm -f "$OBJECTSTORE_CONFIG" "$BACKUP_CONFIG" |
There was a problem hiding this comment.
This rm -f command is redundant because the cp commands on the following lines will overwrite the destination files if they exist. You can safely remove this line to make the script slightly cleaner.
There was a problem hiding this comment.
Pull request overview
This PR adds AWS S3 synchronization support for configuration files in the cliproxyapi service, extends backup functionality to include configuration files, introduces a new config.template.yaml file, and standardizes model naming from 'glm-4-7' to 'glm-4.7' in fish shell functions.
Changes:
- Added S3 sync for config files to objectstore and backup paths, mirroring the existing auth file sync pattern
- Created config.template.yaml as a centralized configuration template with placeholder substitution
- Standardized GLM-4.7 model name from hyphenated to dot notation in OpenCode fish functions
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| home-manager/services/cliproxyapi/scripts/start.sh | Added config directory creation and S3 sync operations for configuration backup to both primary and backup objectstore paths |
| home-manager/programs/fish/functions/_ocxeh_function.fish | Updated model reference from 'cliproxyapi/glm-4-7' to 'cliproxyapi/glm-4.7' for consistency with config alias |
| home-manager/programs/fish/functions/_ocxe_function.fish | Updated model reference from 'cliproxyapi/glm-4-7' to 'cliproxyapi/glm-4.7' for consistency with config alias |
| config/cliproxyapi/default.nix | Changed symlink sources from config.yaml to config.template.yaml for both template and example configurations |
| config/cliproxyapi/config.template.yaml | New comprehensive configuration template with placeholders for secrets, provider configurations, and model mappings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Keep objectstore-backed config in sync for management UI | ||
| OBJECTSTORE_CONFIG_DIR="$CONFIG_DIR/objectstore/config" | ||
| OBJECTSTORE_CONFIG="$OBJECTSTORE_CONFIG_DIR/config.yaml" | ||
| BACKUP_CONFIG_DIR="$CONFIG_DIR/backup/config" | ||
| BACKUP_CONFIG="$BACKUP_CONFIG_DIR/config.yaml" | ||
| mkdir -p "$OBJECTSTORE_CONFIG_DIR" "$BACKUP_CONFIG_DIR" | ||
| rm -f "$OBJECTSTORE_CONFIG" "$BACKUP_CONFIG" | ||
| cp "$CONFIG" "$OBJECTSTORE_CONFIG" | ||
| cp "$CONFIG" "$BACKUP_CONFIG" | ||
|
|
||
| # Push config to objectstore so remote-backed config doesn't revert locally | ||
| if [ -n "$OBJECTSTORE_ENDPOINT" ] && [ -n "$OBJECTSTORE_ACCESS_KEY" ] && [ -n "$OBJECTSTORE_SECRET_KEY" ]; then | ||
| AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$OBJECTSTORE_ENDPOINT" \ | ||
| --no-progress \ | ||
| "$OBJECTSTORE_CONFIG_DIR/" \ | ||
| "s3://${OBJECTSTORE_BUCKET}/config/" || true | ||
|
|
||
| AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$OBJECTSTORE_ENDPOINT" \ | ||
| --no-progress \ | ||
| "$BACKUP_CONFIG_DIR/" \ | ||
| "s3://${OBJECTSTORE_BUCKET}/backup/config/" || true | ||
| fi |
There was a problem hiding this comment.
The new config synchronization functionality lacks test coverage. The existing test suite in spec/cliproxyapi_spec.sh covers configuration generation and platform-specific handling, but doesn't test the new objectstore config sync operations (directory creation, file copying, and S3 sync behavior). Consider adding tests to verify: 1) directories are created correctly, 2) config files are copied to the objectstore and backup directories, 3) S3 sync is attempted when credentials are present, and 4) the script continues gracefully when S3 sync fails (due to '|| true').
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Changes
Technical Details
Testing
Generated with opencode by glm-4.7
Summary by cubic
Keeps the management UI config in sync across environments by pushing to an S3-compatible object store and saving a local backup. Adds a config template for predictable bootstrapping and fixes the model name in helper scripts.
New Features
Bug Fixes
Written for commit c8e20c2. Summary will update on new commits.