Support configuration env variable replacement #385
Merged
Conversation
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
We want to support environment variable replacement in the configuration similarly to how it is done in llama-stack. Example use case / motivation: The config, which since 3f7ed75 can contain a password to a database, requires users to move their entire config to a Kubernetes secret, as it now contains sensitive information, rather than a configmap. If instead we supported environment variable replacement, users could keep their config in a configmap and just set the password in a secret as k8s allows you to load secret values as environment variables in pods. Since users of lightspeed-stack are already familiar with the syntax for environment variable replacement from llama-stack, we can use the same function from llama-stack to handle this.
Contributor
WalkthroughAdds environment variable substitution during configuration loading by invoking replace_env_vars on the YAML-loaded config dict within AppConfig.load_configuration. No changes to public interfaces or other behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppConfig
participant YAMLLoader
participant EnvSubst as replace_env_vars
participant Model as Configuration
User->>AppConfig: load_configuration(path)
AppConfig->>YAMLLoader: load YAML
YAMLLoader-->>AppConfig: config_dict
AppConfig->>EnvSubst: replace_env_vars(config_dict)
EnvSubst-->>AppConfig: config_dict (resolved)
AppConfig->>Model: instantiate(Configuration, dict)
Model-->>AppConfig: configuration instance
AppConfig-->>User: AppConfig with resolved config
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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: 1
🧹 Nitpick comments (1)
src/configuration.py (1)
6-9: llama-stack dependency pinned—confirm replace_env_vars API stability
- pyproject.toml (lines 28–29) declares and pins
- llama-stack==0.2.17
- llama-stack-client==0.2.17
- src/configuration.py
- line 8:
from llama_stack.core.stack import replace_env_vars- line 46: uses
replace_env_vars(config_dict)Ensure that
replace_env_varsis part of llama-stack’s stable, public API. If it isn’t guaranteed to remain stable, introduce a thin wrapper in this repo to isolate future upstream changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/configuration.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
Comment on lines
43
to
49
| """Load configuration from YAML file.""" | ||
| with open(filename, encoding="utf-8") as fin: | ||
| config_dict = yaml.safe_load(fin) | ||
| config_dict = replace_env_vars(config_dict) | ||
| logger.info("Loaded configuration: %s", config_dict) | ||
| self.init_from_dict(config_dict) | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop logging full configuration (secrets leak) and harden env-var substitution handling.
Now that secrets can come from env vars, logging the entire config at info level risks leaking credentials. Also, yaml.safe_load can return None on empty files; passing that into replace_env_vars will likely error.
- Do not log the full configuration.
- Handle None from yaml.safe_load.
- Add defensive error handling around replace_env_vars to fail fast with a clear message.
Apply this diff:
with open(filename, encoding="utf-8") as fin:
- config_dict = yaml.safe_load(fin)
- config_dict = replace_env_vars(config_dict)
- logger.info("Loaded configuration: %s", config_dict)
+ config_dict = yaml.safe_load(fin) or {}
+ try:
+ config_dict = replace_env_vars(config_dict)
+ except Exception as e:
+ logger.error("Failed to apply environment variable substitution: %s", e)
+ raise
+ # Avoid logging secrets; log only the top-level sections/keys.
+ logger.info("Loaded configuration sections: %s", list(config_dict.keys()))
self.init_from_dict(config_dict)Optional follow-ups I can help with:
- Add unit tests covering nested env-var replacement, missing variables behavior, and type casting to ensure values still validate against Configuration. Want me to draft these?
📝 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
| """Load configuration from YAML file.""" | |
| with open(filename, encoding="utf-8") as fin: | |
| config_dict = yaml.safe_load(fin) | |
| config_dict = replace_env_vars(config_dict) | |
| logger.info("Loaded configuration: %s", config_dict) | |
| self.init_from_dict(config_dict) | |
| """Load configuration from YAML file.""" | |
| with open(filename, encoding="utf-8") as fin: | |
| config_dict = yaml.safe_load(fin) or {} | |
| try: | |
| config_dict = replace_env_vars(config_dict) | |
| except Exception as e: | |
| logger.error("Failed to apply environment variable substitution: %s", e) | |
| raise | |
| # Avoid logging secrets; log only the top-level sections/keys. | |
| logger.info("Loaded configuration sections: %s", list(config_dict.keys())) | |
| self.init_from_dict(config_dict) |
🤖 Prompt for AI Agents
In src/configuration.py around lines 43 to 49, stop logging the entire
configuration (to avoid leaking secrets), handle yaml.safe_load possibly
returning None, and add defensive error handling around replace_env_vars to fail
fast with a clear message; specifically, treat a None result from safe_load as
an empty dict (or raise a clear error), wrap the call to replace_env_vars in a
try/except that catches exceptions and raises/ logs a concise ValueError with
context (but do not include config contents or secret values), and replace the
info-level log of the full config with a non-sensitive message (e.g.,
"configuration loaded" or a debug log of a sanitized summary) before calling
self.init_from_dict.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
We want to support environment variable replacement in the configuration similarly to how it is done in llama-stack.
Example use case / motivation:
The config, which since 3f7ed75 can contain a password to a database, requires users to move their entire config to a Kubernetes secret, as it now contains sensitive information, rather than a configmap. If instead we supported environment variable replacement, users could keep their config in a configmap and just set the password in a secret as k8s allows you to load secret values as environment variables in pods.
Since users of lightspeed-stack are already familiar with the syntax for environment variable replacement from llama-stack, we can use the same function from llama-stack to handle this.
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Checked that it works. Since it's a borrowed function, testing seems unnecessary
Summary by CodeRabbit