Skip to content

Commit 0a386e9

Browse files
fix(cli): resolve env var templates before validation in list-deps (#5914)
# What does this PR do? `ogx stack list-deps` passed raw YAML directly to `StackConfig()`, causing Pydantic validation errors for non-string fields using env var syntax (e.g. `registry_refresh_interval_seconds` set to `${env.REGISTRY_REFRESH_INTERVAL_SECONDS:=300}`). Use `replace_env_vars()` before validation, matching the server startup path. This also subsumes the manual `auth` `provider_config` nulling workaround since `replace_env_vars` already handles that. Distributions like `nvidia` and `postgres-demo` use bare `${env.INFERENCE_MODEL}` (no default value) in `registered_resources`. Since these env vars are not set at build/CI time, `replace_env_vars()` would raise `EnvVarError`. The new `ignore_unresolved` parameter makes `list-deps` tolerant of these by resolving bare unset env vars to empty strings, which triggers the existing resource-skip logic. Server startup behavior is unchanged (still raises on missing env vars). ## Test plan - [x] Unit tests for `replace_env_vars` pass (37 tests including 4 new `ignore_unresolved` tests) - [x] Unit tests for `list-deps` pass (14 tests) - [x] `ogx stack list-deps nvidia` succeeds without `INFERENCE_MODEL` set - [x] `ogx stack list-deps postgres-demo` succeeds without `INFERENCE_MODEL` set - [x] Pre-commit checks pass on all changed files - [x] `mypy` passes **Upstream issue**: opendatahub-io/ogx-distribution#429 --------- Signed-off-by: Nathan Weinberg <nweinber@redhat.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 6f981ba commit 0a386e9

3 files changed

Lines changed: 59 additions & 18 deletions

File tree

src/ogx/cli/stack/_list_deps.py

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
from ogx.core.build import get_provider_dependencies
1515
from ogx.core.datatypes import StackConfig
1616
from ogx.core.distribution import get_provider_registry
17-
from ogx.core.stack import run_config_from_dynamic_config_spec
17+
from ogx.core.stack import replace_env_vars, run_config_from_dynamic_config_spec
1818
from ogx.log import get_logger
1919

2020
from .utils import add_dependent_providers
@@ -90,15 +90,7 @@ def run_stack_list_deps_command(args: argparse.Namespace) -> None:
9090
with open(config_file) as f:
9191
try:
9292
contents = yaml.safe_load(f)
93-
# Remove auth provider_config to avoid validation errors with env var syntax.
94-
# We only need provider dependencies, not auth config (auth has no pip_packages).
95-
# This is simpler than modifying the schema to accept type="" which would require
96-
# removing discriminated union and adding custom validation logic and modifying
97-
# all 4 auth provider config classes (a very invasive change)
98-
if "server" in contents and "auth" in contents["server"]:
99-
if "provider_config" in contents["server"]["auth"]:
100-
contents["server"]["auth"]["provider_config"] = None
101-
config = StackConfig(**contents)
93+
config = StackConfig(**replace_env_vars(contents, ignore_unresolved=True))
10294
except Exception as e:
10395
cprint(
10496
f"Could not parse config file {config_file}: {e}",

src/ogx/core/stack.py

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -482,8 +482,15 @@ def _collect(obj: Any, acc: list[str]) -> None:
482482
return result
483483

484484

485-
def replace_env_vars(config: Any, path: str = "") -> Any:
486-
"""Recursively replace environment variable references in a configuration object."""
485+
def replace_env_vars(config: Any, path: str = "", ignore_unresolved: bool = False) -> Any:
486+
"""Recursively replace environment variable references in a configuration object.
487+
488+
When *ignore_unresolved* is True, bare ``${env.VAR}`` references that cannot
489+
be resolved (env var not set, no default) are replaced with an empty string
490+
instead of raising :class:`EnvVarError`. This is used by ``list-deps`` which
491+
only needs the structural shape of the config (provider types), not actual
492+
runtime values.
493+
"""
487494
if isinstance(config, dict):
488495
# Special handling for auth provider_config with conditional type field
489496
# This allows auth to be enabled/disabled via environment variables
@@ -493,15 +500,17 @@ def replace_env_vars(config: Any, path: str = "") -> Any:
493500
if isinstance(provider_cfg, dict) and "type" in provider_cfg:
494501
try:
495502
# Resolve the type field first to check if auth should be enabled
496-
resolved_type = replace_env_vars(provider_cfg["type"], f"{path}.provider_config.type")
503+
resolved_type = replace_env_vars(
504+
provider_cfg["type"], f"{path}.provider_config.type", ignore_unresolved
505+
)
497506

498507
# If type is empty/None, disable auth by setting provider_config to None
499508
# This prevents validation errors on the discriminated union
500509
if resolved_type is None or resolved_type == "":
501510
# Process rest of config normally but exclude provider_config from expansion
502511
# to avoid EnvVarError from bare env vars (e.g., ${env.KEYCLOAK_URL})
503512
result = {
504-
k: replace_env_vars(v, f"{path}.{k}" if path else k)
513+
k: replace_env_vars(v, f"{path}.{k}" if path else k, ignore_unresolved)
505514
for k, v in config.items()
506515
if k != "provider_config"
507516
}
@@ -518,7 +527,7 @@ def replace_env_vars(config: Any, path: str = "") -> Any:
518527
result = {}
519528
for k, v in config.items():
520529
try:
521-
result[k] = replace_env_vars(v, f"{path}.{k}" if path else k)
530+
result[k] = replace_env_vars(v, f"{path}.{k}" if path else k, ignore_unresolved)
522531
except EnvVarError as e:
523532
raise EnvVarError(e.var_name, e.path) from None
524533
return result
@@ -533,7 +542,9 @@ def replace_env_vars(config: Any, path: str = "") -> Any:
533542
# is disabled so that we can skip config env variable expansion and avoid validation errors
534543
if isinstance(v, dict) and "provider_id" in v:
535544
try:
536-
resolved_provider_id = replace_env_vars(v["provider_id"], f"{path}[{i}].provider_id")
545+
resolved_provider_id = replace_env_vars(
546+
v["provider_id"], f"{path}[{i}].provider_id", ignore_unresolved
547+
)
537548
if resolved_provider_id == "__disabled__":
538549
logger.debug(
539550
"Skipping config env variable expansion for disabled provider",
@@ -551,7 +562,9 @@ def replace_env_vars(config: Any, path: str = "") -> Any:
551562
for id_field in RESOURCE_ID_FIELDS:
552563
if id_field in v:
553564
try:
554-
resolved_id = replace_env_vars(v[id_field], f"{path}[{i}].{id_field}")
565+
resolved_id = replace_env_vars(
566+
v[id_field], f"{path}[{i}].{id_field}", ignore_unresolved
567+
)
555568
if resolved_id is None or resolved_id == "":
556569
logger.debug(
557570
"Skipping [] with empty (conditional env var not set)",
@@ -575,7 +588,7 @@ def replace_env_vars(config: Any, path: str = "") -> Any:
575588

576589
# Normal processing
577590
# result is a list here, but mypy sees it could be dict/str
578-
result.append(replace_env_vars(v, f"{path}[{i}]")) # type: ignore[attr-defined]
591+
result.append(replace_env_vars(v, f"{path}[{i}]", ignore_unresolved)) # type: ignore[attr-defined]
579592
except EnvVarError as e:
580593
raise EnvVarError(e.var_name, e.path) from None
581594
return result
@@ -620,6 +633,8 @@ def get_env_var(match: re.Match):
620633
value = ""
621634
else: # No operator case: ${env.FOO}
622635
if not env_value:
636+
if ignore_unresolved:
637+
return ""
623638
raise EnvVarError(env_var, path)
624639
value = env_value
625640

tests/unit/server/test_replace_env_vars.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,40 @@ def test_simple_replacement_raises_when_not_set(setup_env_vars):
4242
assert exc_info.value.var_name == "NOT_SET"
4343

4444

45+
def test_ignore_unresolved_returns_empty_for_bare_env_var(setup_env_vars):
46+
result = replace_env_vars("${env.NOT_SET}", ignore_unresolved=True)
47+
assert result is None
48+
49+
50+
def test_ignore_unresolved_still_resolves_set_vars(setup_env_vars):
51+
assert replace_env_vars("${env.TEST_VAR}", ignore_unresolved=True) == "test_value"
52+
53+
54+
def test_ignore_unresolved_skips_resource_with_bare_env_var(setup_env_vars):
55+
"""Bare ${env.VAR} in a model_id should skip the item when ignore_unresolved=True."""
56+
data = {
57+
"models": [
58+
{"model_id": "${env.INFERENCE_MODEL}", "provider_id": "nvidia"},
59+
{"model_id": "always-present", "provider_id": "other"},
60+
]
61+
}
62+
result = replace_env_vars(data, ignore_unresolved=True)
63+
assert len(result["models"]) == 1
64+
assert result["models"][0]["model_id"] == "always-present"
65+
66+
67+
def test_ignore_unresolved_preserves_defaults_and_conditionals(setup_env_vars):
68+
data = {
69+
"url": "${env.BASE_URL:=https://example.com}",
70+
"key": "${env.API_KEY:=}",
71+
"opt": "${env.OPTIONAL:+enabled}",
72+
}
73+
result = replace_env_vars(data, ignore_unresolved=True)
74+
assert result["url"] == "https://example.com"
75+
assert result["key"] is None
76+
assert result["opt"] is None
77+
78+
4579
def test_default_value_when_not_set(setup_env_vars):
4680
assert replace_env_vars("${env.NOT_SET:=default}") == "default"
4781

0 commit comments

Comments
 (0)