-
Notifications
You must be signed in to change notification settings - Fork 83
refactor(package): Standardize how configuration and credentials are passed to containers (resolves #1149). #1152
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
Changes from 77 commits
b143237
6cddeda
f9d5b59
489476f
0e3f412
05283d5
34c5e8f
842efac
05715fb
87d07c6
73334ed
866334d
dff9bef
00ceeb8
cc9a7c7
4e1c272
7d82bec
1b7c70c
781bb16
bf63d6e
469bc65
8654a56
03881cc
0c9a035
6f8879c
3850b2d
96735b1
365b5d6
c84836c
04c5c98
33e11ac
6e06740
6ef77dd
36af52d
d85085b
e371aa8
5fa14f8
9eba523
312445e
335f25e
3cc3d44
02f393d
8bcae09
4a9bf99
fc27205
40e387d
ed58dcc
89f001f
95b55ef
fa81a9d
4743566
0a8de41
cbdd9fb
67d048a
f852ca4
0acb960
d21efbf
d446e52
c0949cc
f723512
38c75ee
a11b314
4124b60
3fe8368
ce143c6
e15547e
4bba4ed
6ab2f80
cd4437d
14e0219
33bc068
ae8f581
0e52cb1
f063bd9
04f81b7
34c430f
31342c3
ca2db62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,11 +9,12 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import typing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import uuid | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from enum import auto | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import List, Optional, Tuple | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Dict, List, Optional, Tuple | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import yaml | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from clp_py_utils.clp_config import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CLP_DEFAULT_CREDENTIALS_FILE_PATH, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CLP_SHARED_CONFIG_FILENAME, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CLPConfig, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DB_COMPONENT_NAME, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| QueryEngine, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -95,6 +96,7 @@ def __init__(self, clp_home: pathlib.Path, docker_clp_home: pathlib.Path): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.archives_output_dir: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.stream_output_dir: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.aws_config_dir: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.generated_config_file: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) LGTM: generated_config_file mount slot added Good addition. Consider marking the file mount read-only when created to prevent accidental in-container edits. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+99
to
100
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Nit: align typing style (Optional over typing.Optional) You already import Optional; keep usage consistent. - self.generated_config_file: typing.Optional[DockerMount] = None
+ self.generated_config_file: Optional[DockerMount] = None📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _validate_data_directory(data_dir: pathlib.Path, component_name: str) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -285,6 +287,18 @@ def generate_container_config( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.stream_output.get_directory(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not is_path_already_mounted( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_home, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CONTAINER_CLP_HOME, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.get_shared_config_file_path(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.get_shared_config_file_path(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docker_mounts.generated_config_file = DockerMount( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DockerMountType.BIND, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.get_shared_config_file_path(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.get_shared_config_file_path(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+290
to
+301
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Mark the shared config file mount read-only The shared config should not be mutated in-container. Make the bind mount ro. - docker_mounts.generated_config_file = DockerMount(
- DockerMountType.BIND,
- clp_config.get_shared_config_file_path(),
- container_clp_config.get_shared_config_file_path(),
- )
+ docker_mounts.generated_config_file = DockerMount(
+ DockerMountType.BIND,
+ clp_config.get_shared_config_file_path(),
+ container_clp_config.get_shared_config_file_path(),
+ True,
+ )📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Only create the mount if the directory exists | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if clp_config.aws_config_directory is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.aws_config_directory = CONTAINER_AWS_CONFIG_DIRECTORY | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -308,33 +322,57 @@ def generate_worker_config(clp_config: CLPConfig) -> WorkerConfig: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return worker_config | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def get_container_config_filename(container_name: str) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return f".{container_name}-config.yml" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def dump_container_config( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config: CLPConfig, clp_config: CLPConfig, container_name: str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Tuple[pathlib.Path, pathlib.Path]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config: CLPConfig, clp_config: CLPConfig, config_filename: str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Writes the given config to the logs directory so that it's accessible in the container. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Writes the given container config to the logs directory, so that it's accessible in the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_clp_config: The config to write. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param clp_config: The corresponding config on the host (used to determine the logs directory). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_name: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param config_filename: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :return: The path to the config file in the container and on the host. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_config_filename = f".{container_name}-config.yml" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config_file_path_on_host = clp_config.logs_directory / container_config_filename | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config_file_path_on_container = container_clp_config.logs_directory / container_config_filename | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config_file_path_on_host = clp_config.logs_directory / config_filename | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config_file_path_on_container = container_clp_config.logs_directory / config_filename | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with open(config_file_path_on_host, "w") as f: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| yaml.safe_dump(container_clp_config.dump_to_primitive_dict(), f) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return config_file_path_on_container, config_file_path_on_host | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def dump_shared_container_config( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config: CLPConfig, clp_config: CLPConfig | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Tuple[pathlib.Path, pathlib.Path]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dumps the given container config to `CLP_SHARED_CONFIG_FILENAME` in the logs directory, so that | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it's accessible in the container. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_clp_config: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param clp_config: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return dump_container_config(container_clp_config, clp_config, CLP_SHARED_CONFIG_FILENAME) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def generate_container_start_cmd( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_name: str, container_mounts: List[Optional[DockerMount]], container_image: str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_name: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_mounts: List[Optional[DockerMount]], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_image: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| extra_env_vars: Optional[Dict[str, str]] = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Generates the command to start a container with the given mounts and name. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Generates the command to start a container with the given mounts, environment variables, and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_name: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_mounts: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_image: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param extra_env_vars: Environment variables to set on top of the predefined ones. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :return: The command. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_site_packages_dir = CONTAINER_CLP_HOME / "lib" / "python3" / "site-packages" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -350,6 +388,12 @@ def generate_container_start_cmd( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--name", container_name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--log-driver", "local" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "PYTHONPATH": clp_site_packages_dir, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the above @coderabbitai create an issue to track the removal of the deplicate
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 42 @junhaoliao, I've created issue #1513 to track the removal of the duplicate PYTHONPATH environment variable definition in 🧬 Code Graph Analysis Results[clp-package-utils/general.py] Summary: generate_container_start_cmd
[clp-package-utils/general.py] Summary: get_common_env_vars_list
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| **(extra_env_vars if extra_env_vars is not None else {}), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for key, value in env_vars.items(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_start_cmd.extend(["-e", f"{key}={value}"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for mount in container_mounts: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if mount: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_start_cmd.append("--mount") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -428,21 +472,21 @@ def validate_and_load_db_credentials_file( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config: CLPConfig, clp_home: pathlib.Path, generate_default_file: bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_credentials_file_path(clp_config, clp_home, generate_default_file) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.load_database_credentials_from_file() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.database.load_credentials_from_file(clp_config.credentials_file_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def validate_and_load_queue_credentials_file( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config: CLPConfig, clp_home: pathlib.Path, generate_default_file: bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_credentials_file_path(clp_config, clp_home, generate_default_file) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.load_queue_credentials_from_file() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.queue.load_credentials_from_file(clp_config.credentials_file_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def validate_and_load_redis_credentials_file( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config: CLPConfig, clp_home: pathlib.Path, generate_default_file: bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_credentials_file_path(clp_config, clp_home, generate_default_file) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.load_redis_credentials_from_file() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.redis.load_credentials_from_file(clp_config.credentials_file_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def validate_db_config(clp_config: CLPConfig, data_dir: pathlib.Path, logs_dir: pathlib.Path): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -599,3 +643,68 @@ def is_retention_period_configured(clp_config: CLPConfig) -> bool: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def get_common_env_vars_list( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| include_clp_home_env_var=True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param include_clp_home_env_var: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :return: A list of common environment variables for Docker containers, in the format | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "KEY=VALUE". | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_site_packages_dir = CONTAINER_CLP_HOME / "lib" / "python3" / "site-packages" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars = [f"PYTHONPATH={clp_site_packages_dir}"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if include_clp_home_env_var: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_HOME={CONTAINER_CLP_HOME}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return env_vars | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def get_credential_env_vars_list( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config: CLPConfig, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| include_db_credentials=False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| include_queue_credentials=False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| include_redis_credentials=False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_clp_config: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param include_db_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param include_queue_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param include_redis_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :return: A list of credential environment variables for Docker containers, in the format | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "KEY=VALUE". | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if include_db_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_DB_USER={container_clp_config.database.username}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_DB_PASS={container_clp_config.database.password}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if include_queue_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_QUEUE_USER={container_clp_config.queue.username}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_QUEUE_PASS={container_clp_config.queue.password}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if include_redis_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_REDIS_PASS={container_clp_config.redis.password}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return env_vars | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def get_celery_connection_env_vars_list(container_clp_config: CLPConfig) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_clp_config: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :return: A list of Celery connection environment variables for Docker containers, in the format | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "KEY=VALUE". | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"BROKER_URL=amqp://" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.queue.username}:{container_clp_config.queue.password}@" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.queue.host}:{container_clp_config.queue.port}", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"RESULT_BACKEND=redis://default:{container_clp_config.redis.password}@" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.redis.host}:{container_clp_config.redis.port}/" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.redis.query_backend_database}", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+701
to
+707
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion URL-encode credentials in Celery connection strings Credentials containing special characters (e.g., Apply URL encoding to credentials before embedding them in connection strings: +from urllib.parse import quote
def get_celery_connection_env_vars_list(container_clp_config: CLPConfig) -> List[str]:
"""
:param container_clp_config:
:return: A list of Celery connection environment variables for Docker containers, in the format
"KEY=VALUE".
"""
+ queue_user = quote(container_clp_config.queue.username, safe='')
+ queue_pass = quote(container_clp_config.queue.password, safe='')
+ redis_pass = quote(container_clp_config.redis.password, safe='')
env_vars = [
f"BROKER_URL=amqp://"
- f"{container_clp_config.queue.username}:{container_clp_config.queue.password}@"
+ f"{queue_user}:{queue_pass}@"
f"{container_clp_config.queue.host}:{container_clp_config.queue.port}",
- f"RESULT_BACKEND=redis://default:{container_clp_config.redis.password}@"
+ f"RESULT_BACKEND=redis://default:{redis_pass}@"
f"{container_clp_config.redis.host}:{container_clp_config.redis.port}/"
f"{container_clp_config.redis.query_backend_database}",
]📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return env_vars | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+695
to
+710
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Celery env: parameterise Redis DB and URL-encode credentials This helper hardcodes the query DB and embeds raw credentials. Parameterise the DB index and encode user/pass to avoid URL parsing issues. -def get_celery_connection_env_vars_list(container_clp_config: CLPConfig) -> List[str]:
+def get_celery_connection_env_vars_list(
+ container_clp_config: CLPConfig,
+ redis_database: Optional[int] = None,
+) -> List[str]:
"""
:param container_clp_config:
:return: A list of Celery connection environment variables for Docker containers, in the format
"KEY=VALUE".
"""
- env_vars = [
- f"BROKER_URL=amqp://"
- f"{container_clp_config.queue.username}:{container_clp_config.queue.password}@"
- f"{container_clp_config.queue.host}:{container_clp_config.queue.port}",
- f"RESULT_BACKEND=redis://default:{container_clp_config.redis.password}@"
- f"{container_clp_config.redis.host}:{container_clp_config.redis.port}/"
- f"{container_clp_config.redis.query_backend_database}",
- ]
+ from urllib.parse import quote
+ q_user = "" if container_clp_config.queue.username is None else quote(container_clp_config.queue.username, safe="")
+ q_pass = "" if container_clp_config.queue.password is None else quote(container_clp_config.queue.password, safe="")
+ r_pass = container_clp_config.redis.password
+ redis_db = (
+ container_clp_config.redis.query_backend_database
+ if redis_database is None
+ else redis_database
+ )
+ broker = (
+ f"amqp://{q_user}:{q_pass}@"
+ f"{container_clp_config.queue.host}:{container_clp_config.queue.port}"
+ )
+ if r_pass:
+ backend = (
+ "redis://default:"
+ f"{quote(r_pass, safe='')}@"
+ f"{container_clp_config.redis.host}:{container_clp_config.redis.port}/{redis_db}"
+ )
+ else:
+ backend = (
+ f"redis://{container_clp_config.redis.host}:{container_clp_config.redis.port}/{redis_db}"
+ )
+ env_vars = [f"BROKER_URL={broker}", f"RESULT_BACKEND={backend}"]
return env_vars📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ | |
| from typing import Final, List, Optional | ||
|
|
||
| from clp_py_utils.clp_config import ( | ||
| CLP_DB_PASS_ENV_VAR_NAME, | ||
| CLP_DB_USER_ENV_VAR_NAME, | ||
| CLP_DEFAULT_DATASET_NAME, | ||
| StorageEngine, | ||
| StorageType, | ||
|
|
@@ -20,6 +22,7 @@ | |
| generate_container_name, | ||
| generate_container_start_cmd, | ||
| get_clp_home, | ||
| get_container_config_filename, | ||
| load_config_file, | ||
| validate_and_load_db_credentials_file, | ||
| validate_dataset_name, | ||
|
|
@@ -212,16 +215,20 @@ def main(argv: List[str]) -> int: | |
|
|
||
| container_clp_config, mounts = generate_container_config(clp_config, clp_home) | ||
| generated_config_path_on_container, generated_config_path_on_host = dump_container_config( | ||
| container_clp_config, clp_config, container_name | ||
| container_clp_config, clp_config, get_container_config_filename(container_name) | ||
| ) | ||
|
|
||
| necessary_mounts: List[Optional[DockerMount]] = [ | ||
| mounts.clp_home, | ||
| mounts.logs_dir, | ||
| mounts.archives_output_dir, | ||
| ] | ||
| extra_env_vars = { | ||
| CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username, | ||
| CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password, | ||
| } | ||
| container_start_cmd: List[str] = generate_container_start_cmd( | ||
| container_name, necessary_mounts, clp_config.execution_container | ||
| container_name, necessary_mounts, clp_config.execution_container, extra_env_vars | ||
| ) | ||
|
Comment on lines
+226
to
232
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Do not pass secrets on the Docker command line; provide values via subprocess env Passing -e KEY=VALUE exposes creds via process args and logs. Pass only the keys to the container CLI and inject the values via subprocess.run env. - extra_env_vars = {
- CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username,
- CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password,
- }
- container_start_cmd: List[str] = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ extra_env_vars = {
+ CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username,
+ CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password,
+ }
+ # Provide only names to the docker CLI; inject values via subprocess env.
+ container_start_cmd: List[str] = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ list(extra_env_vars.keys()),
+ )Apply outside-range updates to complete the change: # At top-level imports
import os # add
# Where the command is executed
proc = subprocess.run(cmd, env={**os.environ, **extra_env_vars}) |
||
|
|
||
| # fmt: off | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ | |
| from typing import List, Optional | ||
|
|
||
| from clp_py_utils.clp_config import ( | ||
| CLP_DB_PASS_ENV_VAR_NAME, | ||
| CLP_DB_USER_ENV_VAR_NAME, | ||
| CLP_DEFAULT_DATASET_NAME, | ||
| StorageEngine, | ||
| ) | ||
|
|
@@ -20,6 +22,7 @@ | |
| generate_container_name, | ||
| generate_container_start_cmd, | ||
| get_clp_home, | ||
| get_container_config_filename, | ||
| JobType, | ||
| load_config_file, | ||
| validate_and_load_db_credentials_file, | ||
|
|
@@ -202,7 +205,7 @@ def main(argv): | |
|
|
||
| container_clp_config, mounts = generate_container_config(clp_config, clp_home) | ||
| generated_config_path_on_container, generated_config_path_on_host = dump_container_config( | ||
| container_clp_config, clp_config, container_name | ||
| container_clp_config, clp_config, get_container_config_filename(container_name) | ||
| ) | ||
|
|
||
| necessary_mounts = [mounts.clp_home, mounts.data_dir, mounts.logs_dir] | ||
|
|
@@ -222,8 +225,12 @@ def main(argv): | |
|
|
||
| _generate_logs_list(clp_config.logs_input.type, container_logs_list_path, parsed_args) | ||
|
|
||
| extra_env_vars = { | ||
| CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username, | ||
| CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password, | ||
| } | ||
| container_start_cmd = generate_container_start_cmd( | ||
| container_name, necessary_mounts, clp_config.execution_container | ||
| container_name, necessary_mounts, clp_config.execution_container, extra_env_vars | ||
| ) | ||
|
Comment on lines
+228
to
234
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainStop leaking DB credentials on the Docker CLI; pass values via subprocess env extra_env_vars currently flows into the docker argv as -e KEY=VALUE, exposing secrets via ps and logs. Emit only -e KEY and inject the values via subprocess env. Action:
Apply within this hunk: - container_start_cmd = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ # NOTE: Requires general.generate_container_start_cmd to emit '-e KEY' (no '=VALUE')
+ container_start_cmd = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ list(extra_env_vars.keys()),
+ )And update the subprocess invocation to inject secrets via env (outside this hunk): # at top of file
import os- subprocess.run(cmd, check=True)
+ subprocess.run(cmd, check=True, env={**os.environ, **extra_env_vars})Run to locate other CLI injections that still embed secrets: 🏁 Script executed: #!/bin/bash
rg -n -C2 -P '\bgenerate_container_start_cmd\s*\(' --glob '!**/venv/**'
rg -n -C2 -P 'append_docker_options\(.+,\s*env_vars\)' --glob '!**/venv/**'Length of output: 9124 Stop leaking DB credentials via Docker CLI flags Exposing secrets with “-e KEY=VALUE” leaks credentials in process listings and logs. To secure this: • Refactor Add at top of each affected file: import osExample diff for compress.py: - extra_env_vars = {
- CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username,
- CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password,
- }
- container_start_cmd = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ extra_env_vars = {
+ CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username,
+ CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password,
+ }
+ # Emit only “-e KEY” and inject values via env
+ container_start_cmd = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ list(extra_env_vars.keys()),
+ )
@@ -240,7 +245,9 @@ def run_compress(...):
- subprocess.run(container_start_cmd, check=True)
+ subprocess.run(
+ container_start_cmd,
+ check=True,
+ env={**os.environ, **extra_env_vars},
+ )🤖 Prompt for AI Agents |
||
| compress_cmd = _generate_compress_cmd( | ||
| parsed_args, dataset, generated_config_path_on_container, logs_list_path_on_container | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ | |
| from typing import Optional | ||
|
|
||
| from clp_py_utils.clp_config import ( | ||
| CLP_DB_PASS_ENV_VAR_NAME, | ||
| CLP_DB_USER_ENV_VAR_NAME, | ||
| CLP_DEFAULT_DATASET_NAME, | ||
| CLPConfig, | ||
| StorageEngine, | ||
|
|
@@ -24,6 +26,7 @@ | |
| generate_container_name, | ||
| generate_container_start_cmd, | ||
| get_clp_home, | ||
| get_container_config_filename, | ||
| JobType, | ||
| load_config_file, | ||
| validate_and_load_db_credentials_file, | ||
|
|
@@ -99,7 +102,7 @@ def handle_extract_file_cmd( | |
| container_name = generate_container_name(str(JobType.FILE_EXTRACTION)) | ||
| container_clp_config, mounts = generate_container_config(clp_config, clp_home) | ||
| generated_config_path_on_container, generated_config_path_on_host = dump_container_config( | ||
| container_clp_config, clp_config, container_name | ||
| container_clp_config, clp_config, get_container_config_filename(container_name) | ||
| ) | ||
|
|
||
| # Set up mounts | ||
|
|
@@ -122,8 +125,13 @@ def handle_extract_file_cmd( | |
| container_paths_to_extract_file_path, | ||
| ) | ||
| ) | ||
|
|
||
| extra_env_vars = { | ||
| CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username, | ||
| CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password, | ||
| } | ||
|
Comment on lines
+129
to
+132
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) DRY: factor out DB env var assembly The extra_env_vars construction is duplicated. Extract a tiny helper to keep it consistent and reduce future drift. Example: def _build_db_env(clp_config: CLPConfig) -> Dict[str, str]:
return {
CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username,
CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password,
}Then: extra_env_vars = _build_db_env(clp_config)Also applies to: 202-205 🤖 Prompt for AI Agents |
||
| container_start_cmd = generate_container_start_cmd( | ||
| container_name, necessary_mounts, clp_config.execution_container | ||
| container_name, necessary_mounts, clp_config.execution_container, extra_env_vars | ||
| ) | ||
|
Comment on lines
+129
to
135
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Do not print DB creds in docker argv; pass via env Emit only the variable names on the docker command line and inject actual values via subprocess env. Apply: - container_start_cmd = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ # NOTE: Requires general.generate_container_start_cmd to emit '-e KEY'
+ container_start_cmd = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ list(extra_env_vars.keys()),
+ )And later (outside this hunk): import os # at top if missing
# ...
subprocess.run(cmd, check=True, env={**os.environ, **extra_env_vars}) |
||
|
|
||
| # fmt: off | ||
|
|
@@ -191,11 +199,15 @@ def handle_extract_stream_cmd( | |
| container_name = generate_container_name(str(JobType.IR_EXTRACTION)) | ||
| container_clp_config, mounts = generate_container_config(clp_config, clp_home) | ||
| generated_config_path_on_container, generated_config_path_on_host = dump_container_config( | ||
| container_clp_config, clp_config, container_name | ||
| container_clp_config, clp_config, get_container_config_filename(container_name) | ||
| ) | ||
| necessary_mounts = [mounts.clp_home, mounts.logs_dir] | ||
| extra_env_vars = { | ||
| CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username, | ||
| CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password, | ||
| } | ||
| container_start_cmd = generate_container_start_cmd( | ||
| container_name, necessary_mounts, clp_config.execution_container | ||
| container_name, necessary_mounts, clp_config.execution_container, extra_env_vars | ||
| ) | ||
|
Comment on lines
+205
to
211
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Apply the same secret-handling fix to the stream extraction path Mirror the recommended approach here as well: pass only names to Docker and set values in subprocess env to avoid leaking credentials via argv. Apply this localized change (after updating the builder as described above): - container_start_cmd = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ container_start_cmd = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ env_var_names=list(extra_env_vars.keys()),
+ )And ensure: env = {**os.environ, **extra_env_vars}
subprocess.run(cmd, check=True, env=env) |
||
|
|
||
| # fmt: off | ||
|
|
||
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.
🧹 Nitpick (assertive)
Unify typing style (avoid mixing
typing.OptionalwithOptional).You import
Optional,List, etc., but still usetyping.Optionalelsewhere. Prefer one style for consistency; since you already import from typing, useOptionalconsistently.Apply this refactor in the class where it appears:
class CLPDockerMounts: def __init__(self, clp_home: pathlib.Path, docker_clp_home: pathlib.Path): - self.input_logs_dir: typing.Optional[DockerMount] = None + self.input_logs_dir: Optional[DockerMount] = None self.clp_home: typing.Optional[DockerMount] = DockerMount( DockerMountType.BIND, clp_home, docker_clp_home ) - self.data_dir: typing.Optional[DockerMount] = None - self.logs_dir: typing.Optional[DockerMount] = None - self.archives_output_dir: typing.Optional[DockerMount] = None - self.stream_output_dir: typing.Optional[DockerMount] = None - self.aws_config_dir: typing.Optional[DockerMount] = None + self.data_dir: Optional[DockerMount] = None + self.logs_dir: Optional[DockerMount] = None + self.archives_output_dir: Optional[DockerMount] = None + self.stream_output_dir: Optional[DockerMount] = None + self.aws_config_dir: Optional[DockerMount] = None📝 Committable suggestion
🤖 Prompt for AI Agents