-
Notifications
You must be signed in to change notification settings - Fork 111
core:services:kraken: Add default ENV vars #3594
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
core:services:kraken: Add default ENV vars #3594
Conversation
Reviewer's GuideThis PR introduces helper methods to centralize container HostConfig/LogConfig setup and inject default host environment variables into Kraken container configurations. Class diagram for updated Kraken extension container config helpersclassDiagram
class Extension {
}
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(config)
KrakenExtension : _set_container_config_host_config(config)
KrakenExtension : async start()
KrakenExtension --> Extension
Class diagram for new DEFAULT_INJECTED_ENV_VARIABLES constantclassDiagram
class KrakenConfig {
+DEFAULT_INJECTED_ENV_VARIABLES: List[str]
}
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ae92f9e
to
fa43a63
Compare
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:
- In _set_container_config_default_env_variables you check for
variable not in config['Env']
but config['Env'] entries are full "KEY=VALUE" strings, so consider matching by prefix (e.g. startswith "KEY=") to avoid duplicate injections. - _set_container_config_host_config currently unconditionally overwrites any existing LogConfig; consider merging with or preserving user-provided log settings instead of always replacing them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In _set_container_config_default_env_variables you check for `variable not in config['Env']` but config['Env'] entries are full "KEY=VALUE" strings, so consider matching by prefix (e.g. startswith "KEY=") to avoid duplicate injections.
- _set_container_config_host_config currently unconditionally overwrites any existing LogConfig; consider merging with or preserving user-provided log settings instead of always replacing them.
## Individual Comments
### Comment 1
<location> `core/services/kraken/extension/extension.py:107-114` </location>
<code_context>
self._settings.extensions.append(extension)
self._manager.save()
+ def _set_container_config_default_env_variables(self, config: Dict[str, Any]) -> None:
+ if "Env" not in config:
+ config["Env"] = []
+
+ for variable in DEFAULT_INJECTED_ENV_VARIABLES:
+ env_val = os.getenv(variable)
+ if variable not in config["Env"] and env_val:
</code_context>
<issue_to_address>
**suggestion:** Consider handling duplicate environment variable entries more robustly.
Currently, the check only looks for the variable name in config["Env"], which may miss cases where the variable exists with a different value. Consider parsing existing entries to ensure no duplicate variable names are added.
```suggestion
def _set_container_config_default_env_variables(self, config: Dict[str, Any]) -> None:
if "Env" not in config:
config["Env"] = []
# Parse existing env variable names
existing_var_names = set()
for entry in config["Env"]:
if "=" in entry:
name, _ = entry.split("=", 1)
existing_var_names.add(name)
else:
existing_var_names.add(entry)
for variable in DEFAULT_INJECTED_ENV_VARIABLES:
env_val = os.getenv(variable)
if variable not in existing_var_names and env_val:
config["Env"].append(f"{variable}={env_val}")
```
</issue_to_address>
### Comment 2
<location> `core/services/kraken/config.py:20-22` </location>
<code_context>
]
-__all__ = ["SERVICE_NAME", "DEFAULT_MANIFESTS", "DEFAULT_EXTENSIONS"]
+DEFAULT_INJECTED_ENV_VARIABLES = [
+ "MAV_SYSTEM_ID",
+]
</code_context>
<issue_to_address>
**suggestion:** Consider documenting the rationale for the default injected environment variables.
Providing context for the default variables will help future maintainers avoid accidental changes and ensure correct usage.
```suggestion
# These environment variables are injected by default into service containers.
# Rationale: MAV_SYSTEM_ID is required for correct identification of the MAVLink system
# in all deployments. Changing or removing this variable may break communication with
# MAVLink-based services. Add additional variables here only if they are universally
# required for all services.
DEFAULT_INJECTED_ENV_VARIABLES = [
"MAV_SYSTEM_ID",
]
```
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fa43a63
to
d7908dd
Compare
* Inject default env vars in containers when starting it
d7908dd
to
dc3c931
Compare
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.
Tested ✔️
Closes #3449
Summary by Sourcery
Add default environment variable injection and log rotation configuration to Kraken Docker container setup.
New Features:
Enhancements: