fix: alb_log regex #1302
fix: alb_log regex #1302
Conversation
WalkthroughThe changes include two main updates: First, the Changes
Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
resources/formats.json (1)
72-113: Verify the intended ALB log regex fix
The PR title indicates a fix to thealb_logpattern, but the diff only reformats the existing regex. Please confirm that this formatted pattern still meets the original requirements—e.g., matching all ALB protocols (http|https|h2|ws|wss), capturing every named group correctly, and handling edge cases. It would be valuable to add a unit or integration test against representative ALB log lines to guard against regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
resources/formats.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
🔇 Additional comments (1)
resources/formats.json (1)
1-1420: Reformatted JSON only; no semantic changes detected
The diff is purely a pretty-print of the existingformats.jsonfile. All entries, regex patterns, capture group names, and field lists remain unchanged in logic and order. The file is valid JSON with consistent two-space indentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Dockerfile.kafka (2)
38-56: Enable strict error handling in the multi-command RUNTo ensure every step in this multi-line
RUNfails immediately and surfaces errors, consider prefixing your shell invocation withset -euxo pipefail. This will make debugging easier and prevent silent failures:RUN set -euxo pipefail && \ mkdir -p src && echo "fn main() {}" > src/main.rs && \ cargo fetch && \ # …
43-50: Improve rdkafka-sys lookup and patch logicThe current approach hardcodes
/usr/local/cargo/registry/srcand only patches the first match. For better portability and to handle multiple crate versions:
- Reference the cargo registry dynamically via
${CARGO_HOME:-/usr/local/cargo}- Restrict
finddepth to speed up the scan- Iterate over all matching directories
Example refactor:
- RDKAFKA_SYS_DIR=$(find /usr/local/cargo/registry/src -name "rdkafka-sys-*" -type d | head -n 1) && \ - sed -i 's/cmake_minimum_required(VERSION 3.2)/cmake_minimum_required(VERSION 3.5)/' "$RDKAFKA_SYS_DIR/librdkafka/CMakeLists.txt" && \ + export CARGO_REG_SRC="${CARGO_HOME:-/usr/local/cargo}/registry/src" + find "$CARGO_REG_SRC" -maxdepth 2 -type d -name "rdkafka-sys-*" | \ + while read -r dir; do + sed -i 's/cmake_minimum_required(VERSION 3.2)/cmake_minimum_required(VERSION 3.5)/' "$dir/librdkafka/CMakeLists.txt" + done && \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile.kafka(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
🔇 Additional comments (1)
Dockerfile.kafka (1)
57-58: Leverage Docker layer caching for dependenciesGreat use of a dummy
main.rsin a separate build step to precompile dependencies. This two-phase build (dependencies first, then real source) maximizes Docker layer cache efficiency, speeding up iterative builds when only application code changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build.yaml (1)
133-133: Remove trailing whitespace
Several newly added lines contain trailing spaces, triggering YAMLlint errors. Please strip the trailing whitespace on lines 133, 138, 142, 145, 148, 151, and 154.Also applies to: 138-138, 142-142, 145-145, 148-148, 151-151, 154-154
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 133-133: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yaml(1 hunks).github/workflows/coverage.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/coverage.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build.yaml
[error] 133-133: trailing spaces
(trailing-spaces)
[error] 138-138: trailing spaces
(trailing-spaces)
[error] 142-142: trailing spaces
(trailing-spaces)
[error] 145-145: trailing spaces
(trailing-spaces)
[error] 148-148: trailing spaces
(trailing-spaces)
[error] 151-151: trailing spaces
(trailing-spaces)
[error] 154-154: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/build.yaml (1)
167-171: Cross-platformsed -isyntax implemented
You’ve correctly applied OS-specific logic forsed -ion macOS vs. Linux, addressing the portability concern raised earlier.
🧹 Nitpick comments (1)
.github/workflows/build.yaml (1)
150-179: Trim trailing whitespace & ensure newline at EOF
YAMLlint reports trailing spaces on lines 150, 152, 156, 159, 162, 165, 172, 179 and a missing newline at end of file. Please remove all trailing spaces and add a final blank line to conform to YAML standards.🧰 Tools
🪛 actionlint (1.7.4)
150-150: this step is for running action since it contains at least one of "uses", "with" keys, but also contains "run" key which is used for running shell command
(syntax-check)
🪛 YAMLlint (1.35.1)
[error] 152-152: trailing spaces
(trailing-spaces)
[error] 156-156: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
[error] 162-162: trailing spaces
(trailing-spaces)
[error] 165-165: trailing spaces
(trailing-spaces)
[error] 172-172: trailing spaces
(trailing-spaces)
[error] 179-179: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yaml
150-150: this step is for running action since it contains at least one of "uses", "with" keys, but also contains "run" key which is used for running shell command
(syntax-check)
🪛 YAMLlint (1.35.1)
.github/workflows/build.yaml
[error] 152-152: trailing spaces
(trailing-spaces)
[error] 156-156: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
[error] 162-162: trailing spaces
(trailing-spaces)
[error] 165-165: trailing spaces
(trailing-spaces)
[error] 172-172: trailing spaces
(trailing-spaces)
[error] 179-179: trailing spaces
(trailing-spaces)
[error] 181-181: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build.yaml (1)
175-175: Add newline at end-of-file.
YAML linters expect a trailing newline to avoidnew-line-at-end-of-fileerrors. Please ensure the file ends with a blank line.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 175-175: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yaml(3 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build.yaml
[error] 175-175: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
🔇 Additional comments (3)
.github/workflows/build.yaml (3)
5-7: Ensure globs are parsed correctly inpaths-ignore.
You moved from quoted to unquoted glob patterns fordocs/**,helm/**, andassets/**while keeping"**.md"quoted. Although YAML supports unquoted simple globs, this inconsistency could be confusing or lead to unexpected behavior. Please verify that the CI is still ignoring those paths as intended, or consider quoting all patterns for uniformity.
119-139: Linux-specific patch step is well-scoped.
Good use ofif: runner.os == 'Linux'to isolate the CMakeLists.txt version bump on Linux runners, complete with a backup, in-place edit, and diff output.
140-159: macOS patch step uses correctsed -i ''syntax.
The macOS variant correctly applies the in-place edit withsed -i '', mirrors the Linux step’s backup-and-diff approach, and is gated byif: runner.os == 'macOS'.
Summary by CodeRabbit