Skip to content

refactor(clp-tdl-package): Move clp_binary_path and s3_credential_env into a shared task::utils module. - #2508

Open
LinZhihao-723 wants to merge 3 commits into
y-scope:mainfrom
LinZhihao-723:tdl-task-utils-mod
Open

refactor(clp-tdl-package): Move clp_binary_path and s3_credential_env into a shared task::utils module.#2508
LinZhihao-723 wants to merge 3 commits into
y-scope:mainfrom
LinZhihao-723:tdl-task-utils-mod

Conversation

@LinZhihao-723

@LinZhihao-723 LinZhihao-723 commented Aug 31, 2026

Copy link
Copy Markdown
Member

Description

clp_binary_path and s3_credential_env are currently private to the compression task, but neither is specific to compression: the former resolves any CLP binary under CLP_HOME/bin, and the latter resolves the AWS credential env vars that any task spawning a CLP binary against S3 needs to forward. The upcoming query tasks need both.

This PR moves the two functions, along with the s3_credential_env_credentials unit test, into a new task::utils module. The move is verbatim: the only edits are the functions' visibility (fn -> pub(super) fn, keeping them internal to task) and the imports that follow them. There's no behaviour change.

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

  • cargo +nightly fmt --all -- --check passes.
  • cargo +nightly clippy --workspace --all-targets -- -D warnings passes.
  • cargo nextest run -p clp-tdl-package passes (10 run, 10 passed), with s3_credential_env_credentials now running as task::utils::tests::s3_credential_env_credentials and the remaining compress.rs tests unchanged.

Summary by CodeRabbit

  • Bug Fixes
    • Improved AWS credential handling when accessing S3, including support for temporary session credentials.
    • Standardized CLP binary path resolution across task operations.
  • Refactor
    • Centralized shared task utilities to improve consistency and reliability.
  • Tests
    • Added coverage for explicit AWS credential handling.

@LinZhihao-723
LinZhihao-723 requested a review from a team as a code owner August 31, 2026 15:12
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 71ef332d-d021-46d8-9bea-60d083df62ab

📥 Commits

Reviewing files that changed from the base of the PR and between 80e2285 and 2ce8882.

📒 Files selected for processing (3)
  • components/clp-tdl-package/src/task/compression/compress.rs
  • components/clp-tdl-package/src/task/mod.rs
  • components/clp-tdl-package/src/task/utils.rs

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

The task package adds shared utilities for CLP binary paths and S3 credential environment variables. The compression flow uses these utilities instead of local implementations.

Changes

Task utility centralization

Layer / File(s) Summary
Add shared task utilities
components/clp-tdl-package/src/task/mod.rs, components/clp-tdl-package/src/task/utils.rs
The task module exposes utils. The new utilities construct CLP binary paths, resolve S3 credentials, and test explicit credentials with a session token.
Use utilities in compression
components/clp-tdl-package/src/task/compression/compress.rs
The compression module imports the shared utilities and removes its local helpers, related imports, and duplicate credential test.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 2ce88

This change only centralizes existing task helpers without changing their behavior, visibility outside the package, credential handling, or subprocess execution. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: davidlion

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: moving clp_binary_path and s3_credential_env into the shared task::utils module.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Some tools did not complete. Review the errors below.

🔧 Clippy (1.97.1)

Clippy execution timed out


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.

@Bill-hbrhbr
Bill-hbrhbr self-requested a review August 31, 2026 19:51
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.

2 participants