add check on "VRL compilation warning"#3345
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates collector cleanup to defer client closure, preserve failure artifact collection, and validate successful-test collector logs for VRL compilation warnings. ChangesCollector cleanup validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Skipping CI for Draft Pull Request. |
|
/test functional-target |
3 similar comments
|
/test functional-target |
|
/test functional-target |
|
/test functional-target |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vparfonov The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vparfonov The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/framework/functional/framework.go (2)
152-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider simplifying the redundant
strings.Containspre-check.The
strings.Containscheck gates thegomega.Expect(...).ToNot(gomega.ContainSubstring(...))assertion, but the assertion alone already handles both the pass and fail cases. The pre-check only adds alog.Infodiagnostic before the guaranteed failure. If the diagnostic log is desired, consider usinggomega.Expectwith a custom failure message instead to reduce the double-check pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/framework/functional/framework.go` around lines 152 - 154, Simplify the VRL warning assertion in the collector log validation by removing the redundant strings.Contains pre-check and its log.Info call. Use the existing gomega.Expect assertion directly, adding a custom failure message only if diagnostic context is needed.
136-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
if/elsefor mutually exclusiveg.Failed()branches.
if g.Failed()andif !g.Failed()are mutually exclusive. Combining them into anif/elseavoids evaluatingg.Failed()twice and makes the intent clearer.♻️ Proposed refactor
if g.Failed() { for _, container := range f.Pod.Spec.Containers { log.Info("Dumping logs for container", "container", container.Name) logs, err := oc.Logs().WithNamespace(f.Namespace).WithPod(f.Pod.Name).WithContainer(container.Name).Run() if err != nil { log.Error(err, "Unable to retrieve logs", "container", container.Name) } if _, err = fmt.Fprintln(f.delayedWriter, logs); err != nil { log.Error(err, "Error writing", "container", container.Name) } } f.delayedWriter.FlushToArtifactsDir(fmt.Sprintf("%s_%d.log", g.FileName(), g.LineNumber())) - } - - if !g.Failed() { + } else { if collectorLogs, err := f.ReadCollectorLogs(); err == nil { if strings.Contains(collectorLogs, "VRL compilation warning") { log.Info("VRL compilation warning detected in collector logs") gomega.Expect(collectorLogs).ToNot(gomega.ContainSubstring("VRL compilation warning")) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/framework/functional/framework.go` around lines 136 - 157, Refactor the adjacent g.Failed() checks into a single if/else structure: keep the container log dumping and artifact flushing in the failed branch, and move the collector-log validation into the else branch. Avoid evaluating g.Failed() twice while preserving both branches’ existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/framework/functional/framework.go`:
- Around line 150-156: Update the ReadCollectorLogs error path in the existing
collector-log validation block to log the returned error when reading logs
fails. Preserve the current VRL warning check for successful reads, but ensure
failures are visible in test output instead of being silently ignored.
---
Nitpick comments:
In `@test/framework/functional/framework.go`:
- Around line 152-154: Simplify the VRL warning assertion in the collector log
validation by removing the redundant strings.Contains pre-check and its log.Info
call. Use the existing gomega.Expect assertion directly, adding a custom failure
message only if diagnostic context is needed.
- Around line 136-157: Refactor the adjacent g.Failed() checks into a single
if/else structure: keep the container log dumping and artifact flushing in the
failed branch, and move the collector-log validation into the else branch. Avoid
evaluating g.Failed() twice while preserving both branches’ existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8abb3771-9d9a-4ece-a57d-6c2fe47b63cd
📒 Files selected for processing (2)
test/framework/functional/framework.gotest/functional/outputs/syslog/rfc5424_test.go
b9e9363 to
364b67c
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/framework/functional/framework.go`:
- Around line 150-155: The VRL warning assertion in the collector-log handling
occurs after the g.Failed() check, so its first failure skips artifact
collection. Update the flow around ReadCollectorLogs and the warning Expect to
perform the warning check before failure-artifact handling, or flush artifacts
immediately before asserting, ensuring FlushToArtifactsDir runs when this
assertion fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7dd6c76b-7042-4aa1-8d61-cb8cbf127b42
📒 Files selected for processing (3)
test/framework/functional/framework.gotest/functional/outputs/syslog/rfc3164_test.gotest/functional/outputs/syslog/rfc5424_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/functional/outputs/syslog/rfc3164_test.go
| if !g.Failed() { | ||
| if collectorLogs, err := f.ReadCollectorLogs(); err != nil { | ||
| log.Error(err, "Unable to read collector logs for VRL warning check") | ||
| } else if strings.Contains(collectorLogs, "VRL compilation warning") { | ||
| log.Info("VRL compilation warning detected in collector logs") | ||
| gomega.Expect(collectorLogs).ToNot(gomega.ContainSubstring("VRL compilation warning")) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Capture failure artifacts when the warning assertion fails.
g.Failed() is evaluated before gomega.Expect. If a VRL warning is the first failure, the assertion marks the test failed after the artifact-dump branch was skipped, so FlushToArtifactsDir is never called. Run the warning check before failure-artifact handling, or explicitly flush artifacts before asserting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/framework/functional/framework.go` around lines 150 - 155, The VRL
warning assertion in the collector-log handling occurs after the g.Failed()
check, so its first failure skips artifact collection. Update the flow around
ReadCollectorLogs and the warning Expect to perform the warning check before
failure-artifact handling, or flush artifacts immediately before asserting,
ensuring FlushToArtifactsDir runs when this assertion fails.
Signed-off-by: Vitalii Parfonov <vparfono@redhat.com>
|
@vparfonov: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Description
This PR add checking what VRL code has no warnings
/cc
/assign
Links
Summary by CodeRabbit