feat(clp-package)!: Refactor S3 log-ingestion configuration to use S3 URL as compression script argument (fixes #842). #852
Conversation
## Walkthrough
This update refactors S3 ingestion and storage configuration across multiple components. It introduces new configuration classes for S3 ingestion, removes and merges redundant classes, and centralizes validation logic. S3 input handling now parses a single S3 URL to extract region, bucket, and key prefix, and error messages and help texts are updated for clarity regarding S3 URL usage. The environment variable setup for AWS authentication is streamlined to use only the authentication object. Documentation is updated to reflect the new S3 URL-based interface and configuration structure for compression tasks.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `components/clp-package-utils/clp_package_utils/scripts/compress.py`<br>`components/clp-package-utils/clp_package_utils/scripts/native/compress.py` | Updated error messages and help texts to clarify S3 URL usage; S3 input handling now parses a single S3 URL to extract region, bucket, and key prefix; S3 configuration is now constructed using parsed URL components and direct AWS authentication. |
| `components/clp-py-utils/clp_py_utils/clp_config.py` | Refactored S3 ingestion and storage configuration classes: added `S3IngestionConfig`, renamed `InputFsStorage` to `FsIngestionConfig`, removed `OutputS3Storage`, merged its logic into `S3Storage`, added root validator for `key_prefix`, and updated type annotations and validation logic accordingly. |
| `components/clp-py-utils/clp_py_utils/s3_utils.py` | Changed `get_credential_env_vars` to accept `AwsAuthentication` directly; added `parse_s3_url` function to extract region, bucket, and key prefix from S3 URLs; updated environment variable logic for S3 input and output handling. |
| `components/job-orchestration/job_orchestration/executor/compress/compression_task.py`<br>`components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py`<br>`components/job-orchestration/job_orchestration/executor/query/fs_search_task.py` | Modified environment variable preparation for S3 input to use only the AWS authentication object instead of the full S3 config. |
| `components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py` | Updated error messages to reference "input URL" instead of "input prefix" for S3 input errors. |
| `docs/src/user-guide/guides-using-object-storage/clp-config.md` | Updated configuration example and documentation to remove `s3_config` block and reference only direct `aws_authentication` under `logs_input`. |
| `docs/src/user-guide/guides-using-object-storage/clp-usage.md` | Updated usage instructions and explanations to require a full S3 URL for compression, clarified parameter descriptions, and revised notes on compression scope and file selection. |
| `components/clp-package-utils/clp_package_utils/general.py` | Updated internal call in `validate_worker_config` from `validate_stream_output_dir()` to `validate_stream_output_config()` to reflect renamed validation method. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant CLI
participant compress.py
participant s3_utils.py
User->>CLI: Run compress with S3 URL
CLI->>compress.py: Parse arguments (paths/S3 URL)
compress.py->>s3_utils.py: parse_s3_url(S3 URL)
s3_utils.py-->>compress.py: region, bucket, key_prefix
compress.py->>CLI: Validate and proceed with compression using parsed S3 detailsPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
docs/src/user-guide/guides-using-object-storage/clp-config.md (1)
102-106:⚠️ Potential issueUpdate note about aws_authentication location
The note still references the old structure where
aws_authenticationwas nested underlogs_input.s3_config, which is no longer accurate after your changes.Apply this update to reflect the new structure:
The code blocks below show `aws_authentication` as a top-level key, but it should be nested under -`logs_input.s3_config`, `archive_output.storage.s3_config`, or `stream_output.storage.s3_config` +`logs_input`, `archive_output.storage.s3_config`, or `stream_output.storage.s3_config` depending on the use case.
🧹 Nitpick comments (9)
components/clp-py-utils/clp_py_utils/s3_utils.py (2)
56-62: Doc-string parameters are stale – update to reflect the new function signature.The doc-string still talks about a
configargument and mentionsS3Config / S3InputConfig, but the function now takes anAwsAuthenticationinstance. This can be misleading for anyone reading the docs or using auto-generated help.- :param config: S3Config or S3InputConfig from which to retrieve credentials. + :param auth: AwsAuthentication object describing how to obtain credentials.
63-64: Unnecessary variable initialisation.
env_vars: Optional[Dict[str, str]] = Noneis assigned but never read before being re-assigned later on. You can drop the initialisation to reduce noise.components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py (1)
115-116: Call site updated correctly, but consider propagating failure context.Passing
s3_config.aws_authenticationmatches the new signature ofget_credential_env_vars– good catch. Note however thatget_credential_env_varscan raise aValueError(e.g., missing profile or env vars)._make_clp_s_command_and_env_varswill translate that intoNone, None, but the caller logs only a generic “Error creating command”, losing the root cause.Consider logging
exas part of the error message so the user can diagnose mis-configured credentials faster.docs/src/user-guide/guides-using-object-storage/clp-usage.md (2)
17-22: Tighten wording of bullet list for smoother readingEach item currently starts with “is”, which feels repetitive and triggered the LanguageTool warning. The diff below varies the verbs and keeps the meaning intact.
-* `<bucket-name>` is the name of the S3 bucket containing your logs. -* `<region-code>` is the AWS region [code][aws-region-codes] for the S3 bucket containing your logs. -* `<prefix>` is the prefix of all logs you wish to compress and must begin with the - `<all-logs-prefix>` value from the [compression IAM policy][compression-iam-policy]. +* `<bucket-name>` – name of the S3 bucket that stores your logs. +* `<region-code>` – AWS region [code][aws-region-codes] where the bucket resides. +* `<prefix>` – prefix of all logs you wish to compress. It **must** start with the + `<all-logs-prefix>` value from the [compression IAM policy][compression-iam-policy].🧰 Tools
🪛 LanguageTool
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...containing your logs. *<region-code>is the AWS region [code][aws-region-codes]...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cket containing your logs. *<prefix>is the prefix of all logs you wish to comp...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
27-30: Small grammar tweak in the limitation noteSubject-verb agreement and brevity can be improved.
-If you wish to compress a single log file, specify the entire path to the log file. However, if -that log file's path is a prefix of another log file's path, then both log files will be compressed +If you wish to compress a single log file, specify its full path. However, if the chosen +file’s path is a prefix of another file’s path, both files will be compressed🧰 Tools
🪛 LanguageTool
[grammar] ~28-~28: Possible subject-verb agreement error detected.
Context: ... path to the log file. However, if that log file's path is a prefix of another log ...(THIS_THAT_AGR)
components/clp-package-utils/clp_package_utils/scripts/native/compress.py (1)
146-150: Clarify error message when multiple URLs are suppliedA concise, actionable message is easier for users scanning CLI output.
- elif len(logs_to_compress) != 1: - raise ValueError(f"Too many URLs: {len(logs_to_compress)} > 1") + elif len(logs_to_compress) != 1: + raise ValueError(f"Expected exactly one S3 URL, got {len(logs_to_compress)}.")components/clp-py-utils/clp_py_utils/clp_config.py (3)
401-404: Redundant local variable indump_to_primitive_dict
dict()already returns a fresh copy; the temporary variable is unnecessary.- d = self.dict() - return d + return self.dict()
457-459: Defaulting to “/” may unintentionally scan the whole filesystemUsing the root directory as the default for
FsIngestionConfig.directoryrisks huge, slow or permission-denied walks if the user forgets to override it. Consider making the field mandatory or defaulting to a safer, package-specific path.
688-714: Streamline profile-auth check to avoid temporary listYou create
auth_configs, append items, then iterate once. A direct generator saves an allocation and a few lines.- auth_configs = [] - if StorageType.S3 == self.logs_input.type: - auth_configs.append(self.logs_input.aws_authentication) - if StorageType.S3 == self.archive_output.storage.type: - auth_configs.append(self.archive_output.storage.s3_config.aws_authentication) - if StorageType.S3 == self.stream_output.storage.type: - auth_configs.append(self.stream_output.storage.s3_config.aws_authentication) - - for auth in auth_configs: + for auth in ( + *( [self.logs_input.aws_authentication] if StorageType.S3 == self.logs_input.type else [] ), + *( [self.archive_output.storage.s3_config.aws_authentication] + if StorageType.S3 == self.archive_output.storage.type else [] ), + *( [self.stream_output.storage.s3_config.aws_authentication] + if StorageType.S3 == self.stream_output.storage.type else [] ), + ):It’s a minor readability/perf tweak—feel free to discard if you prefer the existing explicit style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
components/clp-package-utils/clp_package_utils/scripts/compress.py(2 hunks)components/clp-package-utils/clp_package_utils/scripts/native/compress.py(2 hunks)components/clp-py-utils/clp_py_utils/clp_config.py(6 hunks)components/clp-py-utils/clp_py_utils/s3_utils.py(5 hunks)components/job-orchestration/job_orchestration/executor/compress/compression_task.py(1 hunks)components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py(1 hunks)components/job-orchestration/job_orchestration/executor/query/fs_search_task.py(1 hunks)components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py(1 hunks)docs/src/user-guide/guides-using-object-storage/clp-config.md(1 hunks)docs/src/user-guide/guides-using-object-storage/clp-usage.md(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py (1)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
get_credential_env_vars(56-92)
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py (1)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
s3_get_object_metadata(247-278)
components/job-orchestration/job_orchestration/executor/compress/compression_task.py (1)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
get_credential_env_vars(56-92)
components/job-orchestration/job_orchestration/executor/query/fs_search_task.py (1)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
get_credential_env_vars(56-92)
components/clp-package-utils/clp_package_utils/scripts/compress.py (1)
components/job-orchestration/job_orchestration/scheduler/job_config.py (1)
InputType(11-13)
components/clp-package-utils/clp_package_utils/scripts/native/compress.py (2)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
parse_s3_url(199-231)components/job-orchestration/job_orchestration/scheduler/job_config.py (1)
S3InputConfig(31-34)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (5)
AwsAuthentication(346-375)S3Storage(425-454)FsStorage(406-422)StorageType(60-62)AwsAuthType(65-69)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
components/clp-py-utils/clp_py_utils/core.py (1)
make_config_path_absolute(24-35)
🪛 LanguageTool
docs/src/user-guide/guides-using-object-storage/clp-usage.md
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...containing your logs. * <region-code> is the AWS region [code][aws-region-codes]...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cket containing your logs. * <prefix> is the prefix of all logs you wish to comp...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~28-~28: Possible subject-verb agreement error detected.
Context: ... path to the log file. However, if that log file's path is a prefix of another log ...
(THIS_THAT_AGR)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (7)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
107-146: Edge-case:logs_input.typecompared againstStorageTypeenum.
clp_config.logs_inputis anS3InputConfig, whosetypecomes from theInputTypeenum, notStorageType. Comparing it toStorageType.S3relies on both enums using the same string literal"s3". While that works today, it is fragile and will silently break if either enum is refactored.Consider normalising the comparison:
if input_storage_needed and InputType.S3.value == clp_config.logs_input.type:or add a helper that converts
InputType➜StorageTypeexplicitly.components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py (1)
137-144: Great consistency update.Switching the wording from “prefix” to “URL” keeps the error message aligned with the new CLI semantics. No further issues spotted here.
components/job-orchestration/job_orchestration/executor/query/fs_search_task.py (1)
80-80: Updated credential handling to match new APIThe code correctly updates the call to
get_credential_env_vars()to pass only theaws_authenticationattribute instead of the entires3_configobject. This change aligns with the refactored function signature ins3_utils.pywhich now expects anAwsAuthenticationobject directly.docs/src/user-guide/guides-using-object-storage/clp-config.md (1)
17-23: Documentation properly simplified for S3 ingestion configurationThe documentation has been appropriately updated to remove the nested
s3_configblock with itsregion_code,bucket, andkey_prefixfields, keeping only theaws_authenticationsection. This properly reflects the code changes that now use a direct S3 URL approach rather than individual configuration fields.components/job-orchestration/job_orchestration/executor/compress/compression_task.py (1)
210-210: Updated credential handling to match new APIThe code correctly updates the call to
get_credential_env_vars()to pass only theaws_authenticationattribute instead of the entireclp_config.inputobject. This change properly aligns with the refactored function signature ins3_utils.pywhich now expects anAwsAuthenticationobject directly.components/clp-package-utils/clp_package_utils/scripts/compress.py (2)
116-116: Updated error message to refer to S3 URLThe error message has been appropriately changed to refer to "URL" instead of "key prefix" to align with the new S3 ingestion approach that uses a full S3 URL.
144-146: Updated help text to clarify S3 URL supportThe help text for the
pathsargument now correctly indicates that it can accept either filesystem paths or an S3 URL, which matches the refactored S3 ingestion configuration.
components/clp-package-utils/clp_package_utils/scripts/native/compress.py
Show resolved
Hide resolved
components/clp-package-utils/clp_package_utils/scripts/compress.py
Outdated
Show resolved
Hide resolved
…s.py Co-authored-by: haiqi96 <14502009+haiqi96@users.noreply.github.com>
Co-authored-by: haiqi96 <14502009+haiqi96@users.noreply.github.com>
Co-authored-by: haiqi96 <14502009+haiqi96@users.noreply.github.com>
Co-authored-by: haiqi96 <14502009+haiqi96@users.noreply.github.com>
Co-authored-by: haiqi96 <14502009+haiqi96@users.noreply.github.com>
|
The code looks good to me.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/src/user-guide/guides-using-object-storage/clp-usage.md (1)
17-20: Document supported S3 URL formats
It would be helpful to mention that both path-style and virtual-hosted–style S3 URLs are supported (e.g.,https://s3.<region-code>.amazonaws.com/<bucket-name>/<prefix>andhttps://<bucket-name>.s3.<region-code>.amazonaws.com/<prefix>).🧰 Tools
🪛 LanguageTool
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cket containing your logs. *<prefix>is the prefix of all logs you wish to comp...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🧹 Nitpick comments (2)
docs/src/user-guide/guides-using-object-storage/clp-usage.md (2)
23-24: Clarify single-URL limitation wording
Consider rephrasing to improve readability, for example:Note: Only one S3 URL is supported per execution; any objects matching its prefix will be compressed.
26-29: Improve readability of prefix-inclusion note
The current sentences are lengthy and repetitive. For instance, you might condense to:Providing a log file path that is also the prefix of other objects (e.g.
logs/syslog) will compress all matching files; this limitation will be addressed in a future release.🧰 Tools
🪛 LanguageTool
[grammar] ~27-~27: Possible subject-verb agreement error detected.
Context: ... path to the log file. However, if that log file's path is a prefix of another log ...(THIS_THAT_AGR)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/clp-py-utils/clp_py_utils/s3_utils.py(5 hunks)docs/src/user-guide/guides-using-object-storage/clp-usage.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/clp-py-utils/clp_py_utils/s3_utils.py
🧰 Additional context used
🪛 LanguageTool
docs/src/user-guide/guides-using-object-storage/clp-usage.md
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cket containing your logs. * <prefix> is the prefix of all logs you wish to comp...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~27-~27: Possible subject-verb agreement error detected.
Context: ... path to the log file. However, if that log file's path is a prefix of another log ...
(THIS_THAT_AGR)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (1)
docs/src/user-guide/guides-using-object-storage/clp-usage.md (1)
14-14: Update usage example to include full S3 URL
The new example correctly shows the required virtual-hosted–style S3 URL format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/src/user-guide/guides-using-object-storage/clp-usage.md (2)
17-21: Explicitly list supported URL formats
The bullet list clearly enumerates both path-style and virtual-hosted–style S3 URLs, matching the refactored CLI behaviour. Consider optionally adding a hyperlink to AWS’s Virtual Hosting documentation for even more clarity.
23-26: Improve readability with comma
Adding a comma before “and” will improve the flow of the sentence:-* `<prefix>` is the prefix of all logs you wish to compress and must begin with the `<all-logs-prefix>` value from the [compression IAM policy][compression-iam-policy]. +* `<prefix>` is the prefix of all logs you wish to compress, and must begin with the `<all-logs-prefix>` value from the [compression IAM policy][compression-iam-policy].🧰 Tools
🪛 LanguageTool
[uncategorized] ~25-~25: Possible missing comma found.
Context: ...is the prefix of all logs you wish to compress and must begin with the<all-logs-pr...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/src/user-guide/guides-using-object-storage/clp-usage.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/user-guide/guides-using-object-storage/clp-usage.md
[uncategorized] ~25-~25: Possible missing comma found.
Context: ...is the prefix of all logs you wish to compress and must begin with the <all-logs-pr...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~33-~33: Possible subject-verb agreement error detected.
Context: ... path to the log file. However, if that log file's path is a prefix of another log ...
(THIS_THAT_AGR)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (2)
docs/src/user-guide/guides-using-object-storage/clp-usage.md (2)
14-14: Clear placeholder usage
The placeholder<URL>correctly indicates that a full S3 URL should be provided as a positional argument to the script.
29-36: Note section is clear
The note about single-URL support and its prefix-matching limitation is concise and informative. No changes needed here.🧰 Tools
🪛 LanguageTool
[grammar] ~33-~33: Possible subject-verb agreement error detected.
Context: ... path to the log file. However, if that log file's path is a prefix of another log ...(THIS_THAT_AGR)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/src/user-guide/guides-using-object-storage/clp-usage.md (1)
17-19: Add links to AWS Virtual-Hosting documentation for URL formats.While the two URL styles are shown, linking to AWS’s official VirtualHosting guide will help users understand path-style vs virtual-hosted style access. For example:
* `<url>` is a URL identifying the logs to compress. It can have one of two formats: * [Virtual-hosted–style](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html#virtual-hosted-style-access) — `https://<bucket-name>.s3.<region-code>.amazonaws.com/<prefix>` * [Path-style](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html#path-style-access) — `https://s3.<region-code>.amazonaws.com/<bucket-name>/<prefix>`
🧹 Nitpick comments (3)
docs/src/user-guide/guides-using-object-storage/clp-usage.md (3)
28-30: Clarify note about single-URL support.The phrase “Compressing from S3 only supports a single URL” could be tightened for readability. For example:
-Compressing from S3 only supports a single URL but will compress any logs that have the given prefix. +Compressing from S3 currently supports exactly one URL per invocation and will compress all logs matching the extracted prefix.
31-35: Streamline and shorten limitation description.This sentence is long and may overwhelm readers. Consider the following rewrite for clarity:
-If you wish to compress a single log file, specify the entire path to the log file. However, if that log file's path is a prefix of another log file's path, then both log files will be compressed (e.g., with two files "logs/syslog" and "logs/syslog.1", a prefix like "logs/syslog" will cause both logs to be compressed). This limitation will be addressed in a future release. +To compress a single file, specify its full path. Note that if this path is also a prefix of another file (for example, "logs/syslog" and "logs/syslog.1"), both will be compressed. We plan to address this limitation in a future release.🧰 Tools
🪛 LanguageTool
[grammar] ~32-~32: Possible subject-verb agreement error detected.
Context: ... path to the log file. However, if that log file's path is a prefix of another log ...(THIS_THAT_AGR)
40-41: Remove unused footnote reference.The
[logs-input-s3-config]link is defined but never used in this document. Either remove lines 40–41 or insert the reference where appropriate to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/src/user-guide/guides-using-object-storage/clp-config.md(1 hunks)docs/src/user-guide/guides-using-object-storage/clp-usage.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/src/user-guide/guides-using-object-storage/clp-config.md
🧰 Additional context used
🪛 LanguageTool
docs/src/user-guide/guides-using-object-storage/clp-usage.md
[grammar] ~32-~32: Possible subject-verb agreement error detected.
Context: ... path to the log file. However, if that log file's path is a prefix of another log ...
(THIS_THAT_AGR)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (macos-latest)
| raise ValueError(f"Too many key prefixes: {len(logs_to_compress)} > 1") | ||
|
|
||
| s3_config = clp_config.logs_input.s3_config | ||
| if len(logs_to_compress) == 0: |
There was a problem hiding this comment.
Is this case possible for InputType.FS as well?
There was a problem hiding this comment.
technically yes, added check for FS flow
| @@ -51,16 +53,14 @@ def _get_session_credentials(aws_profile: Optional[str] = None) -> Optional[S3Cr | |||
| ) | |||
|
|
|||
|
|
|||
| def get_credential_env_vars(config: S3Config) -> Dict[str, str]: | |||
| def get_credential_env_vars(auth: AwsAuthentication) -> Dict[str, str]: | |||
| """ | |||
| Generates AWS credential environment variables for tasks. | |||
| :param config: S3Config or S3InputConfig from which to retrieve credentials. | |||
There was a problem hiding this comment.
Need to update the docstring as well.
| clp_config.archive_output.storage, | ||
| clp_config.stream_output.storage, | ||
| ] | ||
| input_storage_needed = True |
There was a problem hiding this comment.
Why's this true for the query scheduler & worker?
There was a problem hiding this comment.
Thanks for catching this. looks like a mistake made in previous PR.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/src/user-guide/guides-using-object-storage/clp-usage.md (1)
17-19: Good addition of supported S3 URL formats.
Listing both virtual-hosted–style (https://<bucket>.s3.<region>.amazonaws.com/<prefix>) and path-style (https://s3.<region>.amazonaws.com/<bucket>/<prefix>) URLs addresses compatibility concerns.
🧹 Nitpick comments (3)
docs/src/user-guide/guides-using-object-storage/clp-usage.md (3)
21-25: Add missing comma for clarity in the<prefix>description.
Rewriting “is the prefix of all logs you wish to compress and must begin with the<all-logs-prefix>…” to “is the prefix of all logs you wish to compress, and must begin with the<all-logs-prefix>…” improves readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...is the prefix of all logs you wish to compress and must begin with the<all-logs-...(AI_HYDRA_LEO_MISSING_COMMA)
28-29: Reword for readability in the note block.
Consider:“Compressing from S3 supports only a single URL, but will compress all logs matching that prefix.”
This flows more naturally and emphasises that the URL is singular while matching may be multiple.
31-33: Clarify ambiguity when a log file path is a prefix of another.
You might explicitly reference the two example files in the sentence, e.g.:“…then both files (
logs/syslogandlogs/syslog.1) will be compressed.”
This makes it crystal-clear which files are affected.🧰 Tools
🪛 LanguageTool
[grammar] ~32-~32: Possible subject-verb agreement error detected.
Context: ... path to the log file. However, if that log file's path is a prefix of another log ...(THIS_THAT_AGR)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/src/user-guide/guides-using-object-storage/clp-config.md(1 hunks)docs/src/user-guide/guides-using-object-storage/clp-usage.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/src/user-guide/guides-using-object-storage/clp-config.md
🧰 Additional context used
🪛 LanguageTool
docs/src/user-guide/guides-using-object-storage/clp-usage.md
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...is the prefix of all logs you wish to compress and must begin with the <all-logs-...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~32-~32: Possible subject-verb agreement error detected.
Context: ... path to the log file. However, if that log file's path is a prefix of another log ...
(THIS_THAT_AGR)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-latest)
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
components/clp-package-utils/clp_package_utils/scripts/native/compress.py
Outdated
Show resolved
Hide resolved
components/clp-package-utils/clp_package_utils/scripts/native/compress.py
Outdated
Show resolved
Hide resolved
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
kirkrodrigues
changed the title
Description
(Depends on #743)
This PR reworks the
logs_inputconfiguration introduced in #788 to remove limitations created by using the existingS3Configclass to specify the ingestion source, specifically the need to specify the source bucket before starting CLP and the fact that ingesting a single file or the entire bucket was impossible. Changes include:region_code,bucket, andkey_prefixfromlogs_inputS3 configuration while keeping authentication details inclp_config.compress.pyto take S3 URL pointing to relevant logs instead of key prefix relative to bucket and prefix configured inclp_config.key_prefixfromS3ConfigtoS3Storageto allow inheritance ofS3ConfigbyS3InputConfig.Checklist
breaking change.
Validation performed
Manually verified ingesting logs from FS and S3 using various authentication options.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor