feat(controlplane): add support for failover s3 bucket in helm chart#2803
Merged
feat(controlplane): add support for failover s3 bucket in helm chart#2803
Conversation
pepol
requested review from
Aenimus,
JivusAyrus,
Noroth,
StarpTech,
thisisnithin and
wilsonrivera
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds conditional S3 failover configuration: new Helm values, Secret and Deployment template entries, README documentation, and TypeScript config builder now expose a top-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
pepol
force-pushed
the
peter/eng-8555-dual-cdn-bucket
branch
2 times, most recently
from
fc759eb to
1417b24
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
helm/cosmo/charts/controlplane/values.yaml (1)
209-209: Makes3FailoverForcePathStyledefault explicit for consistency.Using
''works via app fallback, but setting it to'true'(like primary S3) makes chart-level behavior explicit and matches the documented intent.♻️ Suggested change
- s3FailoverForcePathStyle: '' + s3FailoverForcePathStyle: 'true'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/cosmo/charts/controlplane/values.yaml` at line 209, Update the Helm chart value for s3FailoverForcePathStyle so its chart-level default is explicit (set to 'true' instead of an empty string) to match primary S3 behavior and documented intent; locate the s3FailoverForcePathStyle entry in the values.yaml and change its default value from '' to 'true' so the chart conveys explicit configuration rather than relying on application fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/bin/get-config.ts`:
- Around line 53-60: The s3StorageFailover block is broken: the ternary for
s3StorageFailover is missing a false branch and several env names are
incorrect/typoed. Update the conditional that sets s3StorageFailover to include
the false branch (e.g., return undefined when S3_FAILOVER_STORAGE_URL is falsy)
and correct the environment variable references inside the object to use
S3_FAILOVER_REGION (not S3_REGION) and S3_FAILOVER_FORCE_PATH_STYLE (not
S3_FAILOVER_FORCE_PATH_STYKE); also ensure the forcePathStyle expression
evaluates the right var and handles undefined correctly (e.g., default true when
the failover env var is undefined).
---
Nitpick comments:
In `@helm/cosmo/charts/controlplane/values.yaml`:
- Line 209: Update the Helm chart value for s3FailoverForcePathStyle so its
chart-level default is explicit (set to 'true' instead of an empty string) to
match primary S3 behavior and documented intent; locate the
s3FailoverForcePathStyle entry in the values.yaml and change its default value
from '' to 'true' so the chart conveys explicit configuration rather than
relying on application fallback.
🪄 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: CHILL
Plan: Pro
Run ID: 5a544279-ec3c-498f-8b9c-762c92e561a7
📒 Files selected for processing (5)
controlplane/src/bin/get-config.tshelm/cosmo/charts/controlplane/README.mdhelm/cosmo/charts/controlplane/templates/deployment.yamlhelm/cosmo/charts/controlplane/templates/secret.yamlhelm/cosmo/charts/controlplane/values.yaml
pepol
force-pushed
the
peter/eng-8555-dual-cdn-bucket
branch
2 times, most recently
from
d07b84f to
0ad05b0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
helm/cosmo/charts/controlplane/values.yaml (1)
200-201: Consider defaulting failover region toautofor parity with primary S3 config.This keeps failover setup behavior aligned with
s3Regionand reduces required overrides for common S3-compatible setups.Suggested adjustment
- s3FailoverRegion: '' + s3FailoverRegion: 'auto'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/cosmo/charts/controlplane/values.yaml` around lines 200 - 201, The s3FailoverRegion value in values.yaml is empty by default causing mismatch with s3Region behavior; set s3FailoverRegion to 'auto' by default (same as s3Region) so failover uses automatic region detection for S3-compatible setups, and update the surrounding comment/documentation in values.yaml to reflect that 'auto' is the default and what it means. Ensure you modify the s3FailoverRegion entry and any README or chart docs that reference it to keep parity with the s3Region setting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/cosmo/charts/controlplane/templates/deployment.yaml`:
- Around line 298-304: The template currently omits S3_FAILOVER_FORCE_PATH_STYLE
when .Values.configuration.s3FailoverForcePathStyle is false; change the
conditional to check for presence instead of truthiness (e.g., use hasKey
.Values.configuration "s3FailoverForcePathStyle" or test for nil) and always
render the env var with the literal value quoted, setting name:
S3_FAILOVER_FORCE_PATH_STYLE and value: {{
.Values.configuration.s3FailoverForcePathStyle | quote }} so explicit false is
emitted as "false".
In `@helm/cosmo/charts/controlplane/values.yaml`:
- Around line 208-209: The YAML default for s3FailoverForcePathStyle contradicts
its docstring: change the value of s3FailoverForcePathStyle from an empty string
to a boolean true (ensure it's a YAML boolean, not a string) so the default
behavior matches the comment; verify any templates or consumers that read
s3FailoverForcePathStyle expect a boolean and update them if they treat empty
string specially.
---
Nitpick comments:
In `@helm/cosmo/charts/controlplane/values.yaml`:
- Around line 200-201: The s3FailoverRegion value in values.yaml is empty by
default causing mismatch with s3Region behavior; set s3FailoverRegion to 'auto'
by default (same as s3Region) so failover uses automatic region detection for
S3-compatible setups, and update the surrounding comment/documentation in
values.yaml to reflect that 'auto' is the default and what it means. Ensure you
modify the s3FailoverRegion entry and any README or chart docs that reference it
to keep parity with the s3Region setting.
🪄 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: CHILL
Plan: Pro
Run ID: 8c2a3277-adcf-45d3-bb05-6b1f26158bb9
📒 Files selected for processing (5)
controlplane/src/bin/get-config.tshelm/cosmo/charts/controlplane/README.mdhelm/cosmo/charts/controlplane/templates/deployment.yamlhelm/cosmo/charts/controlplane/templates/secret.yamlhelm/cosmo/charts/controlplane/values.yaml
✅ Files skipped from review due to trivial changes (2)
- controlplane/src/bin/get-config.ts
- helm/cosmo/charts/controlplane/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/cosmo/charts/controlplane/templates/secret.yaml
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2803 +/- ##
===========================================
+ Coverage 46.22% 64.32% +18.10%
===========================================
Files 1045 311 -734
Lines 139820 44308 -95512
Branches 8792 4767 -4025
===========================================
- Hits 64633 28502 -36131
+ Misses 73422 15783 -57639
+ Partials 1765 23 -1742
🚀 New features to boost your workflow:
|
Noroth
reviewed
Apr 29, 2026
Noroth
approved these changes
Apr 29, 2026
pepol
force-pushed
the
peter/eng-8555-dual-cdn-bucket
branch
from
0ad05b0 to
b6f9bcc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/cosmo/charts/controlplane/README.md`:
- Line 57: Update the README entry for the Helm chart property
configuration.s3FailoverForcePathStyle to use the phrasing "path-style URLs"
(hyphenate "path-style" and capitalize "URLs") in its description; locate the
table row containing configuration.s3FailoverForcePathStyle and replace the
current description text so it reads something like "Forces usage of path-style
URLs for the failover S3. Default is true." ensuring the exact property name and
default remain unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: 562e7ee8-268e-4fc0-87fb-2caad242dd22
📒 Files selected for processing (5)
controlplane/src/bin/get-config.tshelm/cosmo/charts/controlplane/README.mdhelm/cosmo/charts/controlplane/templates/deployment.yamlhelm/cosmo/charts/controlplane/templates/secret.yamlhelm/cosmo/charts/controlplane/values.yaml
✅ Files skipped from review due to trivial changes (1)
- helm/cosmo/charts/controlplane/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/cosmo/charts/controlplane/templates/secret.yaml
pepol
force-pushed
the
peter/eng-8555-dual-cdn-bucket
branch
from
b6f9bcc to
64cd4cb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Deployment
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.