-
Notifications
You must be signed in to change notification settings - Fork 111
[backport] core:services:kraken: Add default ENV vars #3597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[backport] core:services:kraken: Add default ENV vars #3597
Conversation
* Inject default env vars in containers when starting it
Reviewer's GuideBackports injection of default environment variables and centralizes container HostConfig log settings by introducing helper methods for configuring container Env and HostConfig, updating the container startup flow accordingly, and exposing the new default Env vars list. Class diagram for updated KrakenExtension methodsclassDiagram
class KrakenExtension {
+_set_container_config_default_env_variables(config: Dict[str, Any])
+_set_container_config_host_config(config: Dict[str, Any])
async start()
}
KrakenExtension : _set_container_config_default_env_variables added
KrakenExtension : _set_container_config_host_config added
KrakenExtension : start modified
Class diagram for new DEFAULT_INJECTED_ENV_VARIABLES in configclassDiagram
class KrakenConfig {
+DEFAULT_INJECTED_ENV_VARIABLES: List[str]
}
KrakenConfig : DEFAULT_INJECTED_ENV_VARIABLES added
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Instead of fully overwriting
HostConfig.LogConfig
, merge new log settings with any existing values to avoid clobbering custom configurations. - Extract the hardcoded log rotation parameters (
max-size
andmax-file
) into configurable constants or settings to make them easier to adjust in the future. - Consider making the list of default environment variables (
DEFAULT_INJECTED_ENV_VARIABLES
) configurable (e.g. via a config file or environment) rather than hardcoding it to improve flexibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of fully overwriting `HostConfig.LogConfig`, merge new log settings with any existing values to avoid clobbering custom configurations.
- Extract the hardcoded log rotation parameters (`max-size` and `max-file`) into configurable constants or settings to make them easier to adjust in the future.
- Consider making the list of default environment variables (`DEFAULT_INJECTED_ENV_VARIABLES`) configurable (e.g. via a config file or environment) rather than hardcoding it to improve flexibility.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This is a backport of #3594 into 1.4
Summary by Sourcery
Backport default environment variable injection and standard log configuration for Kraken extension containers
New Features:
Enhancements: