Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jianlinliu 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 |
WalkthroughAdded two debugging log statements to print parameter values in the egressIP networking test file, enhancing diagnostic output for troubleshooting without altering control flow or logic. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/networking/egressip.go`:
- Around line 457-458: The debug line should be replaced with a concise,
type-safe log that removes the XXXTESTXXX marker, avoids printing the non-string
CLI object `oc` with %s, and stops dumping large objects; instead log scalar
fields with explicit verbs and small summaries for complex objects (e.g. log
`externalNamespace`, `probePodName`, `routeName`, `targetProtocol`,
`targetHost`, `targetPort`, `numberOfRequestsToSend` using %s/%d/%v as
appropriate), omit or replace `oc` with a small identifier (e.g. omit it or log
a stable field if available), and replace `packetSnifferDaemonSet`/`egressIPSet`
with limited signals such as their names or lengths (e.g. `.Name` or `len(...)`)
so CI logs remain low-noise.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/extended/networking/egressip.go
test/extended/networking/egressip.go
Outdated
| framework.Logf("XXXTESTXXX oc: %s, externalNamespace: %s, probePodName: %s, routeName: %s, targetProtocol: %s, targetHost: %s, targetPort: %d, numberOfRequestsToSend: %d, packetSnifferDaemonSet: %v, egressIPSet: %v", oc, externalNamespace, probePodName, routeName, targetProtocol, targetHost, targetPort, numberOfRequestsToSend, packetSnifferDaemonSet, egressIPSet) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find CLI type and whether it implements String() string.
echo "=== Searching for CLI type definition ==="
rg -nP --type=go 'type\s+CLI\s+struct'
echo ""
echo "=== Searching for String() method on *CLI ==="
rg -nP --type=go 'func\s*\(\s*(\w+\s+)?\*CLI\s*\)\s*String\s*\(\s*\)\s*string'
echo ""
echo "=== Locating XXXTESTXXX debug marker ==="
rg -n --type=go 'XXXTESTXXX'
echo ""
echo "=== Looking for similar %s usage with oc in Logf ==="
rg -nP --type=go 'Logf\([^)]*%s[^)]*,\s*oc\b'Repository: openshift/origin
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if the file exists and read lines around 457-458
echo "=== File: test/extended/networking/egressip.go (lines 450-465) ==="
if [ -f "test/extended/networking/egressip.go" ]; then
sed -n '450,465p' test/extended/networking/egressip.go | cat -n
else
echo "File not found"
fi
echo ""
echo "=== CLI struct definition (test/extended/util/client.go:85-110) ==="
sed -n '85,110p' test/extended/util/client.go | cat -n
echo ""
echo "=== Search for String() methods on any CLI types ==="
rg -n --type=go 'func.*String\(\).*string' test/extended/util/client.go
echo ""
echo "=== All occurrences of XXXTESTXXX in the repository ==="
rg -r 'XXXTESTXXX' --type=goRepository: openshift/origin
Length of output: 2864
Replace the temporary debug log with type-safe, low-noise fields
Line 457 uses %s for oc (*exutil.CLI), which doesn't implement String() string and will render as malformed output instead of being debuggable. The log also includes a temporary XXXTESTXXX marker and dumps large objects (packetSnifferDaemonSet, egressIPSet) that inflate CI logs without adding signal.
Suggested cleanup
- framework.Logf("XXXTESTXXX oc: %s, externalNamespace: %s, probePodName: %s, routeName: %s, targetProtocol: %s, targetHost: %s, targetPort: %d, numberOfRequestsToSend: %d, packetSnifferDaemonSet: %v, egressIPSet: %v", oc, externalNamespace, probePodName, routeName, targetProtocol, targetHost, targetPort, numberOfRequestsToSend, packetSnifferDaemonSet, egressIPSet)
+ packetSnifferDaemonSetName := "<nil>"
+ if packetSnifferDaemonSet != nil {
+ packetSnifferDaemonSetName = packetSnifferDaemonSet.Name
+ }
+ framework.Logf("Debug egressIP traffic params: externalNamespace=%s probePodName=%s routeName=%s protocol=%s targetHost=%s targetPort=%d requests=%d packetSnifferDaemonSet=%s egressIPSet=%v",
+ externalNamespace, probePodName, routeName, targetProtocol, targetHost, targetPort, numberOfRequestsToSend, packetSnifferDaemonSetName, egressIPSet)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/networking/egressip.go` around lines 457 - 458, The debug line
should be replaced with a concise, type-safe log that removes the XXXTESTXXX
marker, avoids printing the non-string CLI object `oc` with %s, and stops
dumping large objects; instead log scalar fields with explicit verbs and small
summaries for complex objects (e.g. log `externalNamespace`, `probePodName`,
`routeName`, `targetProtocol`, `targetHost`, `targetPort`,
`numberOfRequestsToSend` using %s/%d/%v as appropriate), omit or replace `oc`
with a small identifier (e.g. omit it or log a stable field if available), and
replace `packetSnifferDaemonSet`/`egressIPSet` with limited signals such as
their names or lengths (e.g. `.Name` or `len(...)`) so CI logs remain low-noise.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/networking/egressip.go (1)
457-457:⚠️ Potential issue | 🟡 MinorRemove transient debug marker and log compact fields only.
Line 457 still includes
XXXTESTXXXand dumps large objects (packetSnifferDaemonSet, fullegressIPSet), which makes CI logs noisy. Please keep scalar fields and compact summaries (daemonset name/counts) instead.Suggested cleanup
- framework.Logf("XXXTESTXXX: externalNamespace: %s, probePodName: %s, routeName: %s, targetProtocol: %s, targetHost: %s, targetPort: %d, numberOfRequestsToSend: %d, packetSnifferDaemonSet: %v, egressIPSet: %v", externalNamespace, probePodName, routeName, targetProtocol, targetHost, targetPort, numberOfRequestsToSend, packetSnifferDaemonSet, egressIPSet) + packetSnifferDaemonSetName := "<nil>" + if packetSnifferDaemonSet != nil { + packetSnifferDaemonSetName = packetSnifferDaemonSet.Name + } + framework.Logf("egressIP debug: externalNamespace=%s probePodName=%s routeName=%s targetProtocol=%s targetHost=%s targetPort=%d numberOfRequestsToSend=%d packetSnifferDaemonSet=%s egressIPCount=%d", + externalNamespace, probePodName, routeName, targetProtocol, targetHost, targetPort, numberOfRequestsToSend, packetSnifferDaemonSetName, len(egressIPSet))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/egressip.go` at line 457, Remove the transient "XXXTESTXXX" marker and avoid dumping large objects in the framework.Logf call in test/extended/networking/egressip.go; instead log only scalar fields (externalNamespace, probePodName, routeName, targetProtocol, targetHost, targetPort, numberOfRequestsToSend) and replace packetSnifferDaemonSet and egressIPSet with compact summaries (e.g., packetSnifferDaemonSet.Name and/or replica count, and egressIPSet length or a comma-joined short list), updating the existing Logf invocation to use those summarized values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/networking/egressip.go`:
- Line 457: Remove the transient "XXXTESTXXX" marker and avoid dumping large
objects in the framework.Logf call in test/extended/networking/egressip.go;
instead log only scalar fields (externalNamespace, probePodName, routeName,
targetProtocol, targetHost, targetPort, numberOfRequestsToSend) and replace
packetSnifferDaemonSet and egressIPSet with compact summaries (e.g.,
packetSnifferDaemonSet.Name and/or replica count, and egressIPSet length or a
comma-joined short list), updating the existing Logf invocation to use those
summarized values.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/extended/networking/egressip.go
|
Scheduling required tests: |
|
/test e2e-aws-ovn-serial-publicnet-1of2 |
|
@jianlinliu: The following test 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. |
2 participants
Summary by CodeRabbit