feat(config): substitute ${VAR} env vars in declarative configs#64
feat(config): substitute ${VAR} env vars in declarative configs#64shreemaan-abhishek wants to merge 1 commit into
Conversation
After parsing a config file, recursively walk every string field
(including nested plugin configs and label values) and replace
\${VAR} with the corresponding environment variable. \\\${VAR}
escapes the substitution and yields a literal \${VAR}. Unset env
vars leave the literal in place rather than collapsing to empty.
Applied in ReadConfigFile so diff, sync, and validate all benefit
without per-command plumbing. Map keys are intentionally not
substituted (matches adc behavior).
📝 WalkthroughWalkthroughThis PR adds ChangesEnvironment Variable Substitution in Config Files
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cmd/config/configutil/env_test.go`:
- Around line 107-119: Add a regression test that ensures map[string]interface{}
entries explicitly set to nil/null are preserved by applyEnvSubstitution: create
a new test (e.g., TestApplyEnvSubstitution_PreserveNullMapValues) that builds a
cfg with a service or arbitrary map field containing a key mapped to nil
(interface{}(nil)), run applyEnvSubstitution(cfg), and assert the key still
exists with a nil value (not removed or converted) — reference
applyEnvSubstitution and use the same test style as
TestApplyEnvSubstitution_UnsetVarKeepsLiteral to locate where to add it.
In `@pkg/cmd/config/configutil/env.go`:
- Around line 109-118: The map-walking code currently does v.SetMapIndex(k,
reflect.ValueOf(newVal)) which passes an invalid reflect.Value when newVal ==
nil and causes the key to be deleted instead of storing a typed nil; update the
reflect.Map branch in walkValue (the MapRange loop that calls walkAny) to detect
when newVal == nil and in that case use reflect.Zero(v.Type().Elem()) to create
a typed nil/zero for the map element type, otherwise continue using
reflect.ValueOf(newVal); this preserves explicit nulls while keeping existing
behavior for non-nil replacements.
🪄 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: 04c6a937-5dba-4547-b014-1a1c4da48159
📒 Files selected for processing (4)
pkg/cmd/config/configutil/configutil.gopkg/cmd/config/configutil/env.gopkg/cmd/config/configutil/env_test.gopkg/cmd/config/sync/sync_test.go
| func TestApplyEnvSubstitution_UnsetVarKeepsLiteral(t *testing.T) { | ||
| // Make sure the variable is genuinely unset for this test. | ||
| require.NoError(t, os.Unsetenv("DEFINITELY_NOT_SET_DAILY_VAR")) | ||
|
|
||
| cfg := &api.ConfigFile{ | ||
| Services: []api.Service{ | ||
| {Name: "before-${DEFINITELY_NOT_SET_DAILY_VAR}-after"}, | ||
| }, | ||
| } | ||
| require.NoError(t, applyEnvSubstitution(cfg)) | ||
|
|
||
| assert.Equal(t, "before-${DEFINITELY_NOT_SET_DAILY_VAR}-after", cfg.Services[0].Name) | ||
| } |
There was a problem hiding this comment.
Add a regression test for explicit null map values.
The suite doesn’t currently assert that map[string]interface{} entries set to null remain present after substitution. A focused test here would catch key deletion regressions in the map traversal path.
Also applies to: 172-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cmd/config/configutil/env_test.go` around lines 107 - 119, Add a
regression test that ensures map[string]interface{} entries explicitly set to
nil/null are preserved by applyEnvSubstitution: create a new test (e.g.,
TestApplyEnvSubstitution_PreserveNullMapValues) that builds a cfg with a service
or arbitrary map field containing a key mapped to nil (interface{}(nil)), run
applyEnvSubstitution(cfg), and assert the key still exists with a nil value (not
removed or converted) — reference applyEnvSubstitution and use the same test
style as TestApplyEnvSubstitution_UnsetVarKeepsLiteral to locate where to add
it.
| case reflect.Map: | ||
| iter := v.MapRange() | ||
| for iter.Next() { | ||
| k := iter.Key() | ||
| val := iter.Value() | ||
| // Map values are not addressable; build a replacement and | ||
| // reassign it under the same key. | ||
| newVal := walkAny(val.Interface()) | ||
| v.SetMapIndex(k, reflect.ValueOf(newVal)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's reflect package, what is the behavior difference in Value.SetMapIndex when the element is (a) reflect.ValueOf(nil) and (b) reflect.Zero(mapElemType)?
💡 Result:
In Go's reflect package, the behavior of Value.SetMapIndex depends on whether the element (the second argument) is the zero Value or a valid Value representing a zero-valued instance of the map's element type [1][2]. (a) reflect.ValueOf(nil) When you call reflect.ValueOf(nil), it returns the zero Value [3]. A zero Value is defined as an invalid Value (its IsValid method returns false) [3]. When this zero Value is passed as the element to SetMapIndex, the function interprets it as a command to delete the entry associated with the specified key from the map [1][2]. This is the standard mechanism in reflection to remove map keys [4]. (b) reflect.Zero(mapElemType) When you call reflect.Zero(mapElemType), it returns a valid Value (IsValid returns true) that represents the zero value for the specified type (e.g., 0 for an int, nil for a pointer, or an empty struct) [3]. When this is passed to SetMapIndex, the function does not delete the key. Instead, it explicitly sets the entry for the specified key to that zero value [1][2]. Summary of Difference The critical difference is that the zero Value (returned by reflect.ValueOf(nil) or reflect.Value{}) triggers a deletion operation, whereas reflect.Zero(mapElemType) triggers an assignment operation, effectively setting the map key to its type's zero value [1][2]. Note that if the map's element type is a reference type (like a pointer, map, or slice) or an interface, reflect.Zero will store a nil value for that type, which remains a stored entry in the map rather than removing the key [3].
Citations:
- 1: https://golang.bg/pkg/reflect/
- 2: https://golang.google.cn/pkg/reflect/
- 3: https://pkg.go.dev/reflect
- 4: https://go101.org/article/reflection.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant parts of env.go around the reported lines.
file="pkg/cmd/config/configutil/env.go"
nl -ba "$file" | sed -n '70,200p'Repository: api7/a7
Length of output: 94
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Also show the full walkAny implementation area if separated further down.
file="pkg/cmd/config/configutil/env.go"
nl -ba "$file" | sed -n '200,340p'Repository: api7/a7
Length of output: 94
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for any comments describing null behavior.
file="pkg/cmd/config/configutil/env.go"
rg -n "null|nil" "$file"Repository: api7/a7
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/cmd/config/configutil/env.go"
nl -ba "$file" | sed -n '70,200p'Repository: api7/a7
Length of output: 94
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/cmd/config/configutil/env.go"
nl -ba "$file" | sed -n '200,340p'Repository: api7/a7
Length of output: 94
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/cmd/config/configutil/env.go"
rg -n "null|nil" "$file"Repository: api7/a7
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/cmd/config/configutil/env.go"
sed -n '90,150p' "$file"Repository: api7/a7
Length of output: 1611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/cmd/config/configutil/env.go"
sed -n '140,210p' "$file"Repository: api7/a7
Length of output: 894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/cmd/config/configutil/env.go"
sed -n '1,110p' "$file" | rg -n "nulls are left untouched|Map keys are NOT substituted"Repository: api7/a7
Length of output: 130
Preserve explicit null map values instead of deleting map entries
walkValue’sreflect.Mapbranch assigns withv.SetMapIndex(k, reflect.ValueOf(newVal)); whennewVal == nil,reflect.ValueOf(nil)is an invalidreflect.Value, andSetMapIndexdeletes the key rather than storing a typed “null”.- This conflicts with the file’s “nulls are left untouched” intent, and
walkAnyreturnsnilforcase nil(and the map walker passes that through directly).
Proposed fix
case reflect.Map:
iter := v.MapRange()
for iter.Next() {
k := iter.Key()
val := iter.Value()
// Map values are not addressable; build a replacement and
// reassign it under the same key.
newVal := walkAny(val.Interface())
- v.SetMapIndex(k, reflect.ValueOf(newVal))
+ if newVal == nil {
+ // Preserve explicit null instead of deleting the key.
+ v.SetMapIndex(k, reflect.Zero(v.Type().Elem()))
+ continue
+ }
+ v.SetMapIndex(k, reflect.ValueOf(newVal))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case reflect.Map: | |
| iter := v.MapRange() | |
| for iter.Next() { | |
| k := iter.Key() | |
| val := iter.Value() | |
| // Map values are not addressable; build a replacement and | |
| // reassign it under the same key. | |
| newVal := walkAny(val.Interface()) | |
| v.SetMapIndex(k, reflect.ValueOf(newVal)) | |
| } | |
| case reflect.Map: | |
| iter := v.MapRange() | |
| for iter.Next() { | |
| k := iter.Key() | |
| val := iter.Value() | |
| // Map values are not addressable; build a replacement and | |
| // reassign it under the same key. | |
| newVal := walkAny(val.Interface()) | |
| if newVal == nil { | |
| // Preserve explicit null instead of deleting the key. | |
| v.SetMapIndex(k, reflect.Zero(v.Type().Elem())) | |
| continue | |
| } | |
| v.SetMapIndex(k, reflect.ValueOf(newVal)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cmd/config/configutil/env.go` around lines 109 - 118, The map-walking
code currently does v.SetMapIndex(k, reflect.ValueOf(newVal)) which passes an
invalid reflect.Value when newVal == nil and causes the key to be deleted
instead of storing a typed nil; update the reflect.Map branch in walkValue (the
MapRange loop that calls walkAny) to detect when newVal == nil and in that case
use reflect.Zero(v.Type().Elem()) to create a typed nil/zero for the map element
type, otherwise continue using reflect.ValueOf(newVal); this preserves explicit
nulls while keeping existing behavior for non-nil replacements.
1 participant
Summary
\${VAR}with the corresponding environment variable.\\\${VAR}escapes the substitution and yields a literal\${VAR}.ReadConfigFileso diff, sync, and validate all benefit without per-command plumbing.Part of the adc → a7 parity gap tracked in
docs/adc-test-parity-plan.md.Test plan
pkg/cmd/config/configutil/env_test.gocovering: service name, escape, plugin-config value, SSL cert/key, nested global rule, unset var, route nested fields, plugin metadata nested value, numbers/bools untouched, string labelsTestReadConfigFile_AppliesSubstitutionend-to-endTestConfigSync_SubstitutesEnvVarsInRequestBodyintegration (RegisterResponder captures resolved values in the PUT body)go test ./pkg/... ./internal/...passesgo vet ./...;gofmt -l .cleanOpen question
Unset-var policy diverges from adc (we keep
\${VAR}literal; adc returns empty string). Argument for ours: silent empty strings have caused production incidents elsewhere (auth headers becoming empty, etc). Argument for theirs: bug-for-bug compat. Happy to flip.Summary by CodeRabbit
Release Notes
New Features
${VAR}environment variable substitution across all supported fields. Escaped syntax\${VAR}preserves literal values. Unset variables remain unchanged.Tests