-
-
Notifications
You must be signed in to change notification settings - Fork 899
feat(ch): optionally disable migrations #2715
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
|
WalkthroughThe change modifies the ClickHouse migration logic in the entrypoint script. It introduces a conditional guard that prevents ClickHouse migrations from running unless CLICKHOUSE_URL is set and SKIP_CLICKHOUSE_MIGRATIONS is not "1". When the skip flag is enabled, a skip message is emitted. Additionally, the script now includes robust GOOSE_DBSTRING construction logic that ensures secure=true is appended to the ClickHouse connection string, with handling for three scenarios: when secure= already exists in the URL, when query parameters are present, or when no query parameters exist. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
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 (2)
docker/scripts/entrypoint.sh (2)
18-28: GOOSE_DBSTRING construction is solid, with minor robustness suggestions.The logic correctly appends
secure=truewhile respecting existing parameter configurations. Consider these optional improvements for maximum robustness:
Use anchored grep pattern to avoid false positives in unlikely but possible edge cases (e.g.,
my_secure_param=value):- if echo "$CLICKHOUSE_URL" | grep -q "secure="; then + if echo "$CLICKHOUSE_URL" | grep -qE '(^|[?&])secure='; thenAdd URL validation (optional): Verify CLICKHOUSE_URL is well-formed before string manipulation, especially if this runs in varied environments.
33-34: Skip message is functional but could be more informative.The message confirms the skip flag is active but doesn't indicate context (e.g., development environment, testing, one-off deployment). Consider adding brief context to help operators understand why migrations were skipped.
- echo "SKIP_CLICKHOUSE_MIGRATIONS=1, skipping ClickHouse migrations." + echo "SKIP_CLICKHOUSE_MIGRATIONS=1, skipping ClickHouse migrations. (Running in no-migration mode)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker/scripts/entrypoint.sh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
docker/scripts/entrypoint.sh (1)
13-13: Conditional logic and control flow look correct.The added condition properly gates migrations based on both CLICKHOUSE_URL presence and the skip flag. The elif/else branches correctly handle all cases: running migrations, skipping explicitly, and skipping when URL is absent. Backward compatibility is maintained since migrations run by default (only skipped when explicitly requested).
Also applies to: 33-37
SKIP_CLICKHOUSE_MIGRATIONS=1to disable