feat(providers): auto-discover suffixed env providers#259
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds support for registering multiple instances of the same provider type via suffixed environment variables (e.g., Changes
Sequence DiagramsequenceDiagram
participant Env as Environment<br/>(os.Environ)
participant Parser as Provider<br/>Config Parser
participant Grouper as Env Var<br/>Grouper
participant Overlay as Overlay<br/>Engine
participant Registry as Provider<br/>Registry
Env->>Parser: Raw env vars
Parser->>Grouper: Scan for prefix match<br/>(e.g., OPENAI_*)
Grouper->>Grouper: Group by suffix<br/>(unsuffixed, east, west)
Grouper->>Overlay: Grouped env values<br/>by field type
Overlay->>Overlay: Phase 1: Apply<br/>unsuffixed vars
Overlay->>Overlay: Phase 2: Apply<br/>suffixed vars<br/>(normalize suffix<br/>to name)
Overlay->>Overlay: Selective overwrite<br/>+ validation<br/>(RequireBaseURL, etc)
Overlay->>Registry: Provider instances<br/>(openai, openai-east,<br/>openai-west, ...)
Registry->>Registry: Resolve & cache
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR extends the provider env-var discovery system to auto-register additional provider instances from suffixed environment variables (e.g. Key changes:
Minor concerns (non-blocking):
Confidence Score: 4/5Safe to merge; all primary paths are tested and the logic is correct — remaining notes are non-blocking P2 style improvements. The implementation is well-structured and handles all critical edge cases (RequireBaseURL, YAML overlay, type-safety guards, Azure API_VERSION). Tests are comprehensive for the intended scenarios. The two non-blocking concerns (os.Environ() allocation in a loop, missing coexistence test) do not affect correctness. internal/providers/config.go (os.Environ() per-type allocation) and internal/providers/config_test.go (coexistence test gap) Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant applyProviderEnvVars
participant collectProviderEnvValues
participant applyUnsuffixed
participant applySuffixed
Caller->>applyProviderEnvVars: raw YAML map + discovery config
applyProviderEnvVars->>applyProviderEnvVars: copy raw into result
loop for each sorted provider type
applyProviderEnvVars->>collectProviderEnvValues: providerType, spec
collectProviderEnvValues->>collectProviderEnvValues: scan os.Environ, group entries
collectProviderEnvValues-->>applyProviderEnvVars: grouped env values
opt base provider group present
applyProviderEnvVars->>applyUnsuffixed: result, providerType, spec, values
applyUnsuffixed->>applyUnsuffixed: findEnvOverlayTarget or create entry
applyUnsuffixed-->>applyProviderEnvVars: result updated
end
loop for each named instance
applyProviderEnvVars->>applySuffixed: result, providerType, spec, name, values
applySuffixed->>applySuffixed: derive target name, overlay or create entry
applySuffixed-->>applyProviderEnvVars: result updated
end
end
applyProviderEnvVars-->>Caller: merged result map
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func collectProviderEnvValues(providerType string, spec DiscoveryConfig) map[string]providerEnvValues { | ||
| groups := make(map[string]providerEnvValues) | ||
| prefix := envPrefix(providerType) | ||
| prefixWithSeparator := prefix + "_" | ||
|
|
||
| for _, entry := range os.Environ() { | ||
| key, value, ok := strings.Cut(entry, "=") | ||
| if !ok || value == "" || !strings.HasPrefix(key, prefixWithSeparator) { | ||
| continue | ||
| } | ||
|
|
||
| targetKey, matched, ambiguous := findEnvOverlayTarget(result, providerType) | ||
| if matched { | ||
| existing := result[targetKey] | ||
| if apiKey != "" { | ||
| existing.APIKey = apiKey | ||
| } | ||
| if explicitBaseURL != "" { | ||
| existing.BaseURL = baseURL | ||
| } else if normalizeResolvedBaseURL(existing.BaseURL) == "" && apiKey != "" && spec.DefaultBaseURL != "" { | ||
| existing.BaseURL = spec.DefaultBaseURL | ||
| } | ||
| if apiVersion != "" { | ||
| existing.APIVersion = apiVersion | ||
| } | ||
| if len(models) > 0 { | ||
| existing.Models = models | ||
| } | ||
| result[targetKey] = existing | ||
| } else if ambiguous { | ||
| suffix, field, ok := parseProviderEnvKey(prefix, key, spec) | ||
| if !ok { | ||
| continue | ||
| } else { | ||
| if spec.RequireBaseURL && explicitBaseURL == "" { | ||
| continue | ||
| } | ||
| result[providerType] = config.RawProviderConfig{ | ||
| Type: providerType, | ||
| APIKey: apiKey, | ||
| BaseURL: baseURL, | ||
| APIVersion: apiVersion, | ||
| Models: models, | ||
| } | ||
| } | ||
|
|
||
| values := groups[suffix] | ||
| switch field { | ||
| case providerEnvFieldAPIKey: | ||
| values.APIKey = value | ||
| case providerEnvFieldBaseURL: | ||
| values.BaseURL = normalizeResolvedBaseURL(value) | ||
| case providerEnvFieldAPIVersion: | ||
| values.APIVersion = value | ||
| case providerEnvFieldModels: | ||
| values.Models = parseCSVEnvList(value) | ||
| } | ||
| groups[suffix] = values | ||
| } | ||
|
|
||
| return result | ||
| for suffix, values := range groups { | ||
| if values.empty() { | ||
| delete(groups, suffix) | ||
| } | ||
| } | ||
|
|
||
| return groups | ||
| } |
There was a problem hiding this comment.
os.Environ() called once per provider type in a hot loop
collectProviderEnvValues is invoked inside the applyProviderEnvVars loop that iterates over every registered provider type. Each call to os.Environ() in Go allocates and copies the entire process environment (a slice of strings). With N registered provider types and M env vars, this is O(N × M) allocations.
A simple fix is to snapshot os.Environ() once in applyProviderEnvVars and pass it down:
func applyProviderEnvVars(raw map[string]config.RawProviderConfig, discovery map[string]DiscoveryConfig) map[string]config.RawProviderConfig {
result := make(map[string]config.RawProviderConfig, len(raw))
maps.Copy(result, raw)
environ := os.Environ() // snapshot once
for _, providerType := range sortedDiscoveryTypes(discovery) {
spec := discovery[providerType]
envGroups := collectProviderEnvValues(providerType, spec, environ)
// ...
}
return result
}For typical deployments (~10 provider types) the impact is small, but the fix is trivial and avoids N redundant environment copies.
| func parseProviderEnvKey(prefix, key string, spec DiscoveryConfig) (string, providerEnvField, bool) { | ||
| rest, ok := strings.CutPrefix(key, prefix+"_") | ||
| if !ok { | ||
| return "", 0, false | ||
| } | ||
|
|
||
| fields := []struct { | ||
| name string | ||
| field providerEnvField | ||
| }{ | ||
| {name: "API_VERSION", field: providerEnvFieldAPIVersion}, | ||
| {name: "BASE_URL", field: providerEnvFieldBaseURL}, | ||
| {name: "API_KEY", field: providerEnvFieldAPIKey}, | ||
| {name: "MODELS", field: providerEnvFieldModels}, | ||
| } | ||
|
|
||
| for _, candidate := range fields { | ||
| if candidate.field == providerEnvFieldAPIVersion && !spec.SupportsAPIVersion { | ||
| continue | ||
| } | ||
| if rest == candidate.name { | ||
| return "", candidate.field, true | ||
| } | ||
| suffix, found := strings.CutSuffix(rest, "_"+candidate.name) | ||
| if found && validProviderEnvSuffix(suffix) { | ||
| return suffix, candidate.field, true | ||
| } | ||
| } | ||
|
|
||
| return "", 0, false |
There was a problem hiding this comment.
parseProviderEnvKey checks fields in order (API_VERSION, BASE_URL, API_KEY, MODELS) and applies strings.CutSuffix(rest, "_"+candidate.name). Because the match is greedy from the right, a key like OPENAI_EAST_MODELS_API_KEY yields suffix EAST_MODELS (field API_KEY) rather than being rejected as unrecognised.
This is correct and safe in practice since no sane env var is named that way, but a short comment near the field list explaining the intentional right-anchored greedy match and ordering would help future maintainers avoid accidentally reordering the candidates list and breaking disambiguation.
2 participants
Summary
Tests
Summary by CodeRabbit
New Features
OPENAI_EAST_API_KEYcreates a provider namedopenai-east), reducing the need for configuration files.Documentation
Tests