Skip to content

Conversation

@junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Nov 19, 2025

Description

This PR moves all docker-compose YAML scripts into etc/docker-compose in the Package and corrects all related references.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

task

cd build/clp-package
./sbin/start-clp.sh # observed the package was started successfully
./sbin/compress.sh --timestamp-key=timestamp ~/samples/postgresql/postgresql.log
./sbin/search.sh 1 # observed results returned
./sbin/stop-clp.sh

# edit etc/clp-config to use absolute paths
# e.g., `logs_directory: /tmp/clp/var/logs`
repeated above steps and observed no failure

Summary by CodeRabbit

  • Refactor
    • Moved runtime compose files into a dedicated nested compose directory and updated all scripts and deployment manifests to reference the new locations.
    • Adjusted several service host-path mounts to a higher-level directory to align filesystem layout.
  • New Features
    • Start script now forwards provided arguments to the runtime invocation.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 19, 2025

Walkthrough

Moved docker-compose files into etc/docker-compose/ and updated all references: controller code, shell scripts, top-level compose includes, and service volume mount host paths in the package deployment compose files.

Changes

Cohort / File(s) Summary
Controller path update
components/clp-package-utils/clp_package_utils/controller.py
_get_docker_file_name now returns etc/docker-compose/docker-compose-base.yaml for DeploymentType.BASE instead of docker-compose-base.yaml.
Shell scripts — runtime compose path
components/package-template/src/sbin/admin-tools/archive-manager.sh, components/package-template/src/sbin/admin-tools/dataset-manager.sh, components/package-template/src/sbin/compress-from-s3.sh, components/package-template/src/sbin/compress.sh, components/package-template/src/sbin/decompress.sh, components/package-template/src/sbin/search.sh, components/package-template/src/sbin/start-clp.sh, components/package-template/src/sbin/stop-clp.sh
Updated docker-compose runtime file path from $CLP_HOME/docker-compose.runtime.yaml to $CLP_HOME/etc/docker-compose/docker-compose.runtime.yaml. start-clp.sh also forwards script arguments to the runtime via "$@".
Top-level compose includes
tools/deployment/package/docker-compose.yaml
Updated included file references to point into etc/docker-compose/ (e.g., docker-compose.utils.yamletc/docker-compose/docker-compose.utils.yaml; service extends target → etc/docker-compose/docker-compose-all.yaml).
Service mounts and compose-all
tools/deployment/package/etc/docker-compose/docker-compose-all.yaml
Changed many host bind-mount sources from ./... to ../../... (clp-config, clp-logs, MySQL, RabbitMQ, Redis, MongoDB, results cache, tmp, web UI mounts, etc.) to reflect new compose file location.

Sequence Diagram(s)

(omitted — changes are path and mount updates, not control-flow or new interactive features)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Areas to check:
    • All shell scripts correctly reference $CLP_HOME/etc/docker-compose/docker-compose.runtime.yaml and preserve quoting/argument passing (notably start-clp.sh forwarding "$@").
    • Relative host paths in docker-compose-all.yaml (../../...) are correct relative to the new etc/docker-compose/ location.
    • Top-level compose include and service extends targets point to the intended files under etc/docker-compose/.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: moving docker-compose YAML files into etc/docker-compose directory and updating all references throughout the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f8311d and 9a47fe4.

📒 Files selected for processing (2)
  • components/clp-package-utils/clp_package_utils/controller.py (1 hunks)
  • tools/deployment/package/etc/docker-compose/docker-compose-all.yaml (7 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2025-10-17T19:59:25.596Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:315-315
Timestamp: 2025-10-17T19:59:25.596Z
Learning: In components/clp-package-utils/clp_package_utils/controller.py, worker log directories (compression_worker, query_worker, reducer) created via `mkdir()` do not need `_chown_paths_if_root()` calls because directories are created with the same owner as the script caller. This differs from infrastructure service directories (database, queue, Redis, results cache) which do require explicit ownership changes.

Applied to files:

  • components/clp-package-utils/clp_package_utils/controller.py
📚 Learning: 2025-10-13T03:32:19.293Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).

Applied to files:

  • tools/deployment/package/etc/docker-compose/docker-compose-all.yaml
📚 Learning: 2025-10-07T07:54:32.427Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-py-utils/clp_py_utils/clp_config.py:47-47
Timestamp: 2025-10-07T07:54:32.427Z
Learning: In components/clp-py-utils/clp_py_utils/clp_config.py, the CONTAINER_AWS_CONFIG_DIRECTORY constant is intentionally set to pathlib.Path("/") / ".aws" (i.e., `/.aws`) rather than a user-specific home directory. This hardcoded path is part of the container orchestration design.

Applied to files:

  • tools/deployment/package/etc/docker-compose/docker-compose-all.yaml
📚 Learning: 2025-08-25T06:32:48.313Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1261
File: docs/src/dev-guide/components-core/manylinux-2-28-deps-install.md:24-24
Timestamp: 2025-08-25T06:32:48.313Z
Learning: In the CLP project documentation, when linking to scripts or other files within the repository, use relative paths (e.g., components/core/tools/scripts/...) rather than commit-pinned GitHub URLs to ensure docs and referenced files always belong to the same commit and stay synchronized.

Applied to files:

  • tools/deployment/package/etc/docker-compose/docker-compose-all.yaml
📚 Learning: 2025-10-20T21:05:30.417Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1413
File: tools/docker-images/clp-package/Dockerfile:22-24
Timestamp: 2025-10-20T21:05:30.417Z
Learning: In the clp repository's Dockerfiles, ENV directives should be consolidated into multi-line ENV statements when possible to reduce image layers. ENV statements should only be split into separate commands when consolidation is not possible due to dependencies (e.g., when later variables must reference earlier ones that need to be set first, or when PATH must be modified sequentially).

Applied to files:

  • tools/deployment/package/etc/docker-compose/docker-compose-all.yaml
⏰ 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). (3)
  • GitHub Check: package-image
  • GitHub Check: lint-check (macos-15)
  • GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
components/clp-package-utils/clp_package_utils/controller.py (1)

859-859: LGTM! Path update aligns with the docker-compose relocation.

The updated path correctly reflects the new location of the docker-compose-base.yaml file in the etc/docker-compose/ subdirectory. Since the path is relative to self._clp_home (used as cwd in the subprocess call at line 808), this change will resolve correctly.

tools/deployment/package/etc/docker-compose/docker-compose-all.yaml (1)

35-35: LGTM! All relative path adjustments are correct.

All bind mount source paths have been consistently updated from ./... to ../../... to account for the docker-compose file's relocation from $CLP_HOME/ to $CLP_HOME/etc/docker-compose/. The path adjustments are mathematically correct—going up two directory levels to reach $CLP_HOME before descending into the target directories.

The changes cover all necessary paths including configuration files, data directories, log directories, and web UI settings, and align with the successful validation testing mentioned in the PR objectives.

Also applies to: 40-40, 60-60, 64-64, 67-67, 115-115, 135-135, 139-139, 142-142, 171-171, 175-175, 178-178, 262-262, 298-298, 302-302

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Ruff (0.14.5)
components/clp-package-utils/clp_package_utils/controller.py

�[1;31mruff failed�[0m
�[1mCause:�[0m Failed to load extended configuration /tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml (/components/clp-package-utils/pyproject.toml extends /tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml)
�[1mCause:�[0m Failed to read /tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml
�[1mCause:�[0m No such file or directory (os error 2)


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as @sitaowang1998 pointed out previously, the paths are now relative to $CLP_HOME/etc/docker-compose/ rather than simply $CLP_HOME. If desired, we can keep docker-compose-all.yaml at $CLP_HOME/ to avoid the relative path changes.

@junhaoliao junhaoliao changed the title refactor(deployment): Move supporting docker-compose YAML scripts into etc/docker-compose. refactor(deployment): Move supporting docker-compose YAML scripts into etc/docker-compose; Standardize MCP service enable/disable mechanism (resolves #1620). Nov 19, 2025
@junhaoliao junhaoliao marked this pull request as ready for review November 19, 2025 21:49
@junhaoliao junhaoliao requested a review from a team as a code owner November 19, 2025 21:49
@junhaoliao junhaoliao marked this pull request as draft November 19, 2025 22:21
# Conflicts:
#	tools/deployment/package/etc/docker-compose/docker-compose-all.yaml
@junhaoliao junhaoliao changed the title refactor(deployment): Move supporting docker-compose YAML scripts into etc/docker-compose; Standardize MCP service enable/disable mechanism (resolves #1620). refactor(deployment): Move supporting docker-compose YAML scripts into etc/docker-compose. Nov 19, 2025
@junhaoliao junhaoliao marked this pull request as ready for review November 19, 2025 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant