feat: add env.VAR_NAME support to observability plugin config fields via EnvVarInput and storage normalization#3577
Draft
BearTS wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds env-var-backed fields end‑to‑end: Zod schema helpers and validation, UI EnvVar inputs and normalization, plugin code resolving values via GetValue(), storage normalization/denormalization, and HTTP handlers mapping unresolved-env errors to 400. ChangesEnvironment-Variable Config Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 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)
Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BearTS
changed the title
env.VAR_NAME support to observability plugin config fields via EnvVarInput and storage normalization
Contributor
Confidence Score: 3/5Not ready to merge without resolving the open breaking-behavior and validation issues in the plugin Init paths and storage normalization. Three open concerns from prior review rounds remain unaddressed: the Telemetry Init guard was widened so a plugin configured with enabled=true and an empty or unresolvable push-gateway URL now hard-fails on startup instead of silently skipping; the OTel ValidateConfig and new Init guard both call GetValue() on CollectorURL, which returns empty string when the referenced env var is absent, rejecting a legitimately-configured env-var reference; and normalizePluginConfigForStorage plus checkEnvVarsResolved are both no-ops when Config is a typed struct rather than map[string]any. plugins/telemetry/main.go (Init guard change), plugins/otel/main.go (ValidateConfig and Init guard), framework/configstore/tables/plugin.go (typed-struct bypass) Important Files Changed
Reviews (3): Last reviewed commit: "feat: add support for env variables on c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 6
🧹 Nitpick comments (1)
plugins/maxim/plugin_test.go (1)
209-246: ⚡ Quick winAdd one unit case for a real
env.*token.These cases only cover
schemas.NewEnvVar("literal"), so the new runtime path that resolvesenv.VAR_NAMEis still untested unless the integration test runs with live credentials. A smallt.Setenv(...)case here would cover the feature this PR is introducing.💡 Example test shape
{ name: "Valid config with both fields", config: Config{ APIKey: *schemas.NewEnvVar("test-api-key"), LogRepoID: *schemas.NewEnvVar("test-repo-id"), }, expectError: false, }, + { + name: "Valid config with env-backed API key", + config: Config{ + APIKey: *schemas.NewEnvVar("env.TEST_MAXIM_API_KEY"), + LogRepoID: *schemas.NewEnvVar(""), + }, + expectError: false, + },t.Setenv("TEST_MAXIM_API_KEY", "test-api-key")🤖 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 `@plugins/maxim/plugin_test.go` around lines 209 - 246, Add a unit case that exercises the env.* runtime path by setting an environment variable with t.Setenv and using schemas.NewEnvVar("env.TEST_MAXIM_API_KEY") (or similar env token) for Config.APIKey in the tests block; ensure the test uses the same validation path as the other cases (checks APIKey.GetValue() or calls Init(&tt.config, logger) as appropriate) so the new runtime resolution code is executed without needing real credentials.
🤖 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 `@framework/configstore/tables/plugin.go`:
- Around line 49-61: The normalization currently requires all three EnvVar keys
(value, env_var, from_env) and len(val)==3 which misses valid sparse EnvVar
objects; update the heuristic in plugin.go to treat any object that has at least
the EnvVar shape as an EnvVar: check hasValue, hasEnvVar, hasFromEnv
individually (remove the len(val) == 3 requirement) and then if from_env (cast
val["from_env"] to bool) and env_var is present (cast to string) return that
env_var, otherwise if value is present (cast to string) return the value; keep
safe type assertions and fall back to returning the original object only when
neither key is usable.
In `@plugins/telemetry/main.go`:
- Around line 406-409: The current guard in the startup path checks both
config.PushGateway.Enabled and that config.PushGateway.PushGatewayURL.GetValue()
!= "" which causes a silent no-op when the URL is an unresolved env var; change
the logic so that whenever config.PushGateway.Enabled is true you always call
plugin.EnablePushGateway(config.PushGateway) (remove the extra GetValue()
non-empty check) so EnablePushGateway can validate the URL and return an error;
update the if-block around plugin.EnablePushGateway in main.go to only check
config.PushGateway != nil && config.PushGateway.Enabled and let
EnablePushGateway surface misconfiguration errors.
- Around line 785-786: Check that both resolved credentials are non-empty before
constructing the BasicAuth pusher: read username :=
config.BasicAuth.Username.GetValue() and password :=
config.BasicAuth.Password.GetValue() and only call pusher =
pusher.BasicAuth(username, password) when username != "" && password != ""; if
one is empty, skip building the pusher (or return/log an error) so you don't
create a pusher that will always fail later.
In `@ui/app/workspace/observability/fragments/maximFormFragment.tsx`:
- Around line 70-76: The new EnvVarInput instances (the ones using
value={field.value} and onChange={field.onChange}) are missing data-testid
attributes; add data-testid attributes following the convention
data-testid="<entity>-<element>-<qualifier>" on both EnvVarInput components (the
one with placeholder "Enter your Maxim API key..." and the other similar
control) so e2e selectors can target them (e.g.,
data-testid="maxim-api-key-input" or similar descriptive names) while keeping
existing props like disabled, maskNonEnvValue, value and onChange intact.
In `@ui/app/workspace/observability/fragments/otelFormFragment.tsx`:
- Around line 153-162: The new EnvVarInput components (used with
form.watch("otel_config.protocol"), hasOtelAccess, field.value, and
field.onChange) are missing data-testid attributes required for stable E2E
selectors; add data-testid attributes to each EnvVarInput instance using the
project naming convention data-testid="<entity>-<element>-<qualifier>" (for
example something like data-testid="otel-env-var-input-url" and
"otel-env-var-input-hostport") so both the input at the current block and the
other instance around lines 336–343 are selectable in tests.
In `@ui/lib/types/schemas.ts`:
- Around line 76-80: normalizeEnvVar currently treats any string like
"env.MY_VAR" as a literal value, breaking round-tripping for env-backed fields;
update the string branch in normalizeEnvVar to detect strings that start with
the "env." prefix and return an EnvVar object with value: "" (or empty),
env_var: the substring after "env.", and from_env: true; otherwise keep the
existing behavior for plain strings. Ensure you only change the typeof v ===
"string" branch in function normalizeEnvVar so stored "env.MY_VAR" values reopen
as env-backed fields.
---
Nitpick comments:
In `@plugins/maxim/plugin_test.go`:
- Around line 209-246: Add a unit case that exercises the env.* runtime path by
setting an environment variable with t.Setenv and using
schemas.NewEnvVar("env.TEST_MAXIM_API_KEY") (or similar env token) for
Config.APIKey in the tests block; ensure the test uses the same validation path
as the other cases (checks APIKey.GetValue() or calls Init(&tt.config, logger)
as appropriate) so the new runtime resolution code is executed without needing
real credentials.
🪄 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: d50fc59b-bda4-4fbc-8448-3f029581b1dc
📒 Files selected for processing (10)
framework/configstore/tables/plugin.goplugins/maxim/main.goplugins/maxim/plugin_test.goplugins/otel/main.goplugins/telemetry/main.goui/app/workspace/observability/fragments/maximFormFragment.tsxui/app/workspace/observability/fragments/otelFormFragment.tsxui/app/workspace/observability/fragments/prometheusFormFragment.tsxui/app/workspace/observability/views/plugins/prometheusView.tsxui/lib/types/schemas.ts
Comment on lines
+49
to
+61
| // Detect an EnvVar object: must have exactly "value", "env_var", and "from_env" keys — no more. | ||
| // This mirrors what schemas.EnvVar serializes to, keeping the heuristic tight. | ||
| _, hasValue := val["value"] | ||
| _, hasEnvVar := val["env_var"] | ||
| _, hasFromEnv := val["from_env"] | ||
| if hasValue && hasEnvVar && hasFromEnv && len(val) == 3 { | ||
| if fromEnv, _ := val["from_env"].(bool); fromEnv { | ||
| envVar, _ := val["env_var"].(string) | ||
| return envVar // "env.VAR_NAME" | ||
| } | ||
| value, _ := val["value"].(string) | ||
| return value | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t require all three EnvVar keys to normalize.
The new heuristic only converts objects with exactly value, env_var, and from_env. That misses valid sparse EnvVar payloads, even though the schema layer accepts those keys as optional. In those cases this hook will persist JSON objects instead of the plain-string format the rest of the PR is standardizing on.
💡 Suggested fix
case map[string]any:
- // Detect an EnvVar object: must have exactly "value", "env_var", and "from_env" keys — no more.
- // This mirrors what schemas.EnvVar serializes to, keeping the heuristic tight.
- _, hasValue := val["value"]
- _, hasEnvVar := val["env_var"]
- _, hasFromEnv := val["from_env"]
- if hasValue && hasEnvVar && hasFromEnv && len(val) == 3 {
+ _, hasValue := val["value"]
+ _, hasEnvVar := val["env_var"]
+ _, hasFromEnv := val["from_env"]
+ isEnvVarShape := len(val) > 0
+ for k := range val {
+ if k != "value" && k != "env_var" && k != "from_env" {
+ isEnvVarShape = false
+ break
+ }
+ }
+ if isEnvVarShape && (hasValue || hasEnvVar || hasFromEnv) {
if fromEnv, _ := val["from_env"].(bool); fromEnv {
envVar, _ := val["env_var"].(string)
return envVar // "env.VAR_NAME"
}
value, _ := val["value"].(string)
return value
}🤖 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 `@framework/configstore/tables/plugin.go` around lines 49 - 61, The
normalization currently requires all three EnvVar keys (value, env_var,
from_env) and len(val)==3 which misses valid sparse EnvVar objects; update the
heuristic in plugin.go to treat any object that has at least the EnvVar shape as
an EnvVar: check hasValue, hasEnvVar, hasFromEnv individually (remove the
len(val) == 3 requirement) and then if from_env (cast val["from_env"] to bool)
and env_var is present (cast to string) return that env_var, otherwise if value
is present (cast to string) return the value; keep safe type assertions and fall
back to returning the original object only when neither key is usable.
Comment on lines
+76
to
+80
| // Normalize a string | EnvVar | undefined to a proper EnvVar object | ||
| export function normalizeEnvVar(v?: string | { value?: string; env_var?: string; from_env?: boolean }): { value: string; env_var: string; from_env: boolean } { | ||
| if (!v) return { value: "", env_var: "", from_env: false }; | ||
| if (typeof v === "string") return { value: v, env_var: "", from_env: false }; | ||
| return { value: v.value ?? "", env_var: v.env_var ?? "", from_env: v.from_env ?? false }; |
Contributor
There was a problem hiding this comment.
Restore env-token round-tripping in normalizeEnvVar.
Saved env-backed values now come back from storage as plain strings like env.MY_VAR, but this helper currently rehydrates them as literal values. That makes persisted env-backed fields reopen in plain-text mode and can trigger the literal URL/API-key validators on edit.
💡 Suggested fix
export function normalizeEnvVar(v?: string | { value?: string; env_var?: string; from_env?: boolean }): { value: string; env_var: string; from_env: boolean } {
if (!v) return { value: "", env_var: "", from_env: false };
- if (typeof v === "string") return { value: v, env_var: "", from_env: false };
+ if (typeof v === "string") {
+ const trimmed = v.trim();
+ if (trimmed.startsWith("env.")) {
+ return { value: "", env_var: trimmed, from_env: true };
+ }
+ return { value: v, env_var: "", from_env: false };
+ }
return { value: v.value ?? "", env_var: v.env_var ?? "", from_env: v.from_env ?? false };
}📝 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.
Suggested change
| // Normalize a string | EnvVar | undefined to a proper EnvVar object | |
| export function normalizeEnvVar(v?: string | { value?: string; env_var?: string; from_env?: boolean }): { value: string; env_var: string; from_env: boolean } { | |
| if (!v) return { value: "", env_var: "", from_env: false }; | |
| if (typeof v === "string") return { value: v, env_var: "", from_env: false }; | |
| return { value: v.value ?? "", env_var: v.env_var ?? "", from_env: v.from_env ?? false }; | |
| // Normalize a string | EnvVar | undefined to a proper EnvVar object | |
| export function normalizeEnvVar(v?: string | { value?: string; env_var?: string; from_env?: boolean }): { value: string; env_var: string; from_env: boolean } { | |
| if (!v) return { value: "", env_var: "", from_env: false }; | |
| if (typeof v === "string") { | |
| const trimmed = v.trim(); | |
| if (trimmed.startsWith("env.")) { | |
| return { value: "", env_var: trimmed, from_env: true }; | |
| } | |
| return { value: v, env_var: "", from_env: false }; | |
| } | |
| return { value: v.value ?? "", env_var: v.env_var ?? "", from_env: v.from_env ?? false }; | |
| } |
🤖 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 `@ui/lib/types/schemas.ts` around lines 76 - 80, normalizeEnvVar currently
treats any string like "env.MY_VAR" as a literal value, breaking round-tripping
for env-backed fields; update the string branch in normalizeEnvVar to detect
strings that start with the "env." prefix and return an EnvVar object with
value: "" (or empty), env_var: the substring after "env.", and from_env: true;
otherwise keep the existing behavior for plain strings. Ensure you only change
the typeof v === "string" branch in function normalizeEnvVar so stored
"env.MY_VAR" values reopen as env-backed fields.
1f5fe0c to
6b36179
Compare
BearTS
force-pushed
the
05-19-feat_add_support_for_env_variables_on_certain_fields_in_plugins
branch
from
6b36179 to
bbf11c2
Compare
|
|
BearTS
force-pushed
the
05-19-feat_add_support_for_env_variables_on_certain_fields_in_plugins
branch
from
bbf11c2 to
134fe71
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
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.
Summary
Plugin configuration fields that previously accepted only plain strings now support
env.VAR_NAMEreferences, allowing sensitive values like API keys, collector URLs, and push gateway credentials to be sourced from environment variables at runtime rather than stored as literals.Changes
APIKeyandLogRepoIDfields changed fromstringtoschemas.EnvVar. All usages updated to call.GetValue().CollectorURLandMetricsEndpointchanged fromstringtoschemas.EnvVar. A missingcollector_urlvalidation guard was added toInit. All usages updated to call.GetValue().PushGatewayURL,BasicAuthConfig.Username, andBasicAuthConfig.Passwordchanged fromstringtoschemas.EnvVar. All usages updated to call.GetValue().normalizePluginConfigForStorageinframework/configstore/tables/plugin.go. This function recursively walks a plugin config map before it is marshaled and encrypted, collapsing any{value, env_var, from_env}shaped objects into plain strings ("env.VAR_NAME"or a literal value). This keeps the stored JSON consistent with the format used byconfig.jsonandProxyConfig.MarshalForStorage.otelConfigSchema,maximConfigSchema, andprometheusConfigSchemafields updated fromz.string()toenvVarSchema. Validation logic updated to useisEnvVarSetinstead of string trimming, and format checks are skipped when a field is sourced from an env var (from_env: true).normalizeEnvVarhelper exported fromschemas.tsto safely coercestring | EnvVar | undefinedinto a properEnvVarobject. Form default values and resets updated to use it.isEnvVarSetexported and used to driveshowBasicAuthstate and basic auth inclusion logic.Inputwith eye-icon buttons replaced with theEnvVarInputcomponent usingmaskNonEnvValue. Collector URL and metrics endpoint fields similarly replaced withEnvVarInput. A note aboutenv.VAR_NAMEsupport added to the OTel headers table description.Type of change
Affected areas
How to test
Environment variable resolution: Configure a plugin with
collector_url: env.OTEL_COLLECTOR_URL(or equivalent), set the environment variable, and verify the plugin initializes using the resolved value rather than the literal string.Storage normalization: Save a plugin config via the UI using an
env.VAR_NAMEreference, then inspect the stored JSON to confirm it contains the plain token string rather than a{value, env_var, from_env}object.Literal values: Confirm that plain literal values (e.g., a real API key) continue to work and are stored as plain strings.
Breaking changes
Plugin
Configstructs for Maxim, OTel, and Telemetry have changed field types fromstringtoschemas.EnvVar. Any code that directly assigns or reads these fields as strings must be updated to useschemas.EnvVarvalues and.GetValue(). Configs stored as plain strings in the database remain compatible due to the normalization hook.Security considerations
Sensitive fields (API keys, passwords, push gateway credentials) are no longer required to be stored as plaintext literals. They can now reference environment variables, reducing the risk of secrets being written to the database. The
normalizePluginConfigForStoragehook ensures that even if the UI submits anEnvVarobject shape, only the resolved token or literal is persisted, preventing unexpected JSON object shapes from reaching the encrypted config column.Checklist
docs/contributing/README.mdand followed the guidelines