feat: make storage providers configurable via envars#2740
Open
feat: make storage providers configurable via envars#2740
Conversation
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
Contributor
|
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 (1)
WalkthroughAdded provider-specific Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
❌ Internal Query Planner CI failed to run. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2740 +/- ##
==========================================
- Coverage 63.46% 63.45% -0.01%
==========================================
Files 251 251
Lines 26767 26767
==========================================
- Hits 16987 16986 -1
- Misses 8414 8417 +3
+ Partials 1366 1364 -2
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/pkg/config/config_test.go (1)
598-718: Add a regression test for non-contiguous provider indexes.Great coverage for contiguous indexes (
0,1). Please also lock in the “stop at first gap” behavior (e.g.,0and2should load only index0) since that behavior is called out in the PR description.✅ Suggested test addition
+func TestStorageProviderFromEnvStopsAtFirstIndexGap(t *testing.T) { + t.Setenv("STORAGE_PROVIDER_S3_0_ID", "s3-zero") + t.Setenv("STORAGE_PROVIDER_S3_0_ENDPOINT", "s3-zero.example.com:9000") + t.Setenv("STORAGE_PROVIDER_S3_0_BUCKET", "bucket-zero") + t.Setenv("STORAGE_PROVIDER_S3_0_ACCESS_KEY", "accessKey0") + t.Setenv("STORAGE_PROVIDER_S3_0_SECRET_KEY", "secretKey0") + t.Setenv("STORAGE_PROVIDER_S3_0_REGION", "us-east-1") + t.Setenv("STORAGE_PROVIDER_S3_0_SECURE", "true") + + // Intentionally skip index 1 + t.Setenv("STORAGE_PROVIDER_S3_2_ID", "s3-two") + t.Setenv("STORAGE_PROVIDER_S3_2_ENDPOINT", "s3-two.example.com:9000") + t.Setenv("STORAGE_PROVIDER_S3_2_BUCKET", "bucket-two") + t.Setenv("STORAGE_PROVIDER_S3_2_ACCESS_KEY", "accessKey2") + t.Setenv("STORAGE_PROVIDER_S3_2_SECRET_KEY", "secretKey2") + t.Setenv("STORAGE_PROVIDER_S3_2_REGION", "eu-west-1") + t.Setenv("STORAGE_PROVIDER_S3_2_SECURE", "false") + + f := createTempFileFromFixture(t, ` +version: "1" +`) + cfg, err := LoadConfig([]string{f}) + require.NoError(t, err) + + require.Len(t, cfg.Config.StorageProviders.S3, 1) + require.Equal(t, "s3-zero", cfg.Config.StorageProviders.S3[0].ID) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/config_test.go` around lines 598 - 718, Add regression tests that verify "stop at first gap" behavior for each provider type by creating new test functions (e.g., TestS3StorageProviderFromEnv_NonContiguousIndexes, TestCDNStorageProviderFromEnv_NonContiguousIndexes, TestRedisStorageProviderFromEnv_NonContiguousIndexes, TestFileSystemStorageProviderFromEnv_NonContiguousIndexes) that set environment variables for index 0 and index 2 (skip index 1), call LoadConfig, and assert that cfg.Config.StorageProviders.<S3|CDN|Redis|FileSystem> has length 1 and contains only the values from index 0; mirror the existing tests' assertions (IDs, endpoints/URLs/URLs list/paths, Secure/ClusterEnabled, etc.) to ensure only the first contiguous block is loaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/pkg/config/config_test.go`:
- Around line 598-718: Add regression tests that verify "stop at first gap"
behavior for each provider type by creating new test functions (e.g.,
TestS3StorageProviderFromEnv_NonContiguousIndexes,
TestCDNStorageProviderFromEnv_NonContiguousIndexes,
TestRedisStorageProviderFromEnv_NonContiguousIndexes,
TestFileSystemStorageProviderFromEnv_NonContiguousIndexes) that set environment
variables for index 0 and index 2 (skip index 1), call LoadConfig, and assert
that cfg.Config.StorageProviders.<S3|CDN|Redis|FileSystem> has length 1 and
contains only the values from index 0; mirror the existing tests' assertions
(IDs, endpoints/URLs/URLs list/paths, Secure/ClusterEnabled, etc.) to ensure
only the first contiguous block is loaded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4e80f63-4cf9-4bb6-af5d-2ac4a5cd06ae
📒 Files selected for processing (2)
router/pkg/config/config.gorouter/pkg/config/config_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs-website/router/configuration.mdx (1)
1094-1094: Split the indexing rules into a short list for better reference readability.Line 1094 contains multiple distinct rules in one paragraph. A short bullet list is easier to scan in reference docs.
Proposed doc edit
-Storage providers are configured as arrays, so environment variables use indexed notation: `STORAGE_PROVIDER_{TYPE}_{INDEX}_{FIELD}`. Indexes must start from `0` and be sequential. If an index is skipped, all indexes after the gap are ignored. See [Storage Providers](/router/storage-providers) for more details. +Storage providers are configured as arrays. + +- Use indexed environment variables: `STORAGE_PROVIDER_{TYPE}_{INDEX}_{FIELD}`. +- Indexes start at `0`. +- Indexes must be sequential. If an index is skipped, all following indexes are ignored. + +See [Storage Providers](/router/storage-providers) for more details.As per coding guidelines, "Use structured lists when presenting multiple distinct items. Do not pack them into a single paragraph."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs-website/router/configuration.mdx` at line 1094, The paragraph describing storage provider indexing should be split into a short, structured list for readability: extract the distinct rules about the environment variable format (STORAGE_PROVIDER_{TYPE}_{INDEX}_{FIELD}), that indexes must start at 0, that indexes must be sequential, and that any skipped index causes following indexes to be ignored, and render those as separate bullet points (with a final line linking to the Storage Providers page for details) instead of one combined sentence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-website/router/configuration.mdx`:
- Around line 1108-1117: Update the docs to reflect the actual defaults and
requirements: change the STORAGE_PROVIDER_S3_{n}_SECURE table row to show
default "false" (fix by ensuring S3StorageProvider.Secure's envDefault is
represented as false), and update the STORAGE_PROVIDER_CDN_{n}_URL row to mark
the field as optional (remove the required check icon) while keeping the default
value "https://cosmo-cdn.wundergraph.com" (reflecting CDNStorageProvider.URL's
envDefault). Ensure the environment variable names and YAML keys remain
unchanged.
---
Nitpick comments:
In `@docs-website/router/configuration.mdx`:
- Line 1094: The paragraph describing storage provider indexing should be split
into a short, structured list for readability: extract the distinct rules about
the environment variable format (STORAGE_PROVIDER_{TYPE}_{INDEX}_{FIELD}), that
indexes must start at 0, that indexes must be sequential, and that any skipped
index causes following indexes to be ignored, and render those as separate
bullet points (with a final line linking to the Storage Providers page for
details) instead of one combined sentence.
🪄 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: d55a8334-597d-4f87-9dab-1e44eb2e7944
📒 Files selected for processing (2)
docs-website/router/configuration.mdxdocs-website/router/storage-providers.mdx
✅ Files skipped from review due to trivial changes (1)
- docs-website/router/storage-providers.mdx
dkorittki
reviewed
Apr 8, 2026
SkArchon
commented
Apr 8, 2026
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
2 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.
This PR adds envar support allowing the configuration of storage providers of envars. It uses the already existing slice / struct semantics provided by the carlos0/env library we use.
The above index will create successful entries. (Note that if you skip an index, only indexes starting from 0 until the index before the skipped index will be included, which means if someone adds index 17 in the above example it then will be ignored and not utilized).
Summary by CodeRabbit
Chores
Tests
Documentation
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.