[WIP] ACM-20048 automation#463
Conversation
Signed-off-by: Spencer McAvey <smcavey@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smcavey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughA new Jest test suite for CollectorConfig webhook validation is added, providing utilities to apply CollectorConfig via Kubernetes CLI ( ChangesCollectorConfig Webhook Validation Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 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 `@tests/api/collectorconfig-webhook.test.js`:
- Line 9: Replace the fixed test namespace with a unique per-run namespace to
prevent CI job collisions: change the constant testNamespace in
tests/api/collectorconfig-webhook.test.js to append a run-unique suffix (e.g.,
Date.now(), process.env.CI_JOB_ID, or crypto.randomUUID()) and update all usages
(the other occurrences around the testNamespace references at the sections
noted) so every test run uses something like `${baseName}-${uniqueSuffix}`
instead of "search-webhook-test".
- Around line 22-27: Replace the fragile execSync shell calls that run `oc` with
execFileSync and add timeouts and maxBuffer to prevent CI hangs: for the `echo
'${yaml}' | oc apply -f -` invocation use execFileSync('oc', ['apply','-f','-'],
{ input: yaml, encoding: 'utf-8', timeout: 30000, maxBuffer: 1024*1024 })
instead of execSync so we pass YAML via the input option (avoiding shell
interpolation); for the cleanup execSync calls that invoke `oc` (the other
occurrences) replace them with execFileSync and include at least timeout: 30000
and maxBuffer: 1024*1024 in their options; preserve the existing catch/result
logic but continue to read e.stderr/e.stdout/e.message as before.
🪄 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: 19abc36b-3637-41cb-b42d-61f134f558f7
📒 Files selected for processing (1)
tests/api/collectorconfig-webhook.test.js
|
|
||
| const squad = require('../../config').get('squadName') | ||
|
|
||
| const testNamespace = 'search-webhook-test' |
There was a problem hiding this comment.
Use a unique namespace per run to avoid cross-job collisions
Line 9 uses a fixed namespace (search-webhook-test). Parallel CI jobs/shards can interfere with each other (create/delete races), causing flakes.
Suggested patch
-const testNamespace = 'search-webhook-test'
+const testNamespace = `search-webhook-test-${process.env.BUILD_ID || process.pid}`Also applies to: 41-45, 51-58
🤖 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 `@tests/api/collectorconfig-webhook.test.js` at line 9, Replace the fixed test
namespace with a unique per-run namespace to prevent CI job collisions: change
the constant testNamespace in tests/api/collectorconfig-webhook.test.js to
append a run-unique suffix (e.g., Date.now(), process.env.CI_JOB_ID, or
crypto.randomUUID()) and update all usages (the other occurrences around the
testNamespace references at the sections noted) so every test run uses something
like `${baseName}-${uniqueSuffix}` instead of "search-webhook-test".
| try { | ||
| const output = execSync(`echo '${yaml}' | oc apply -f - 2>&1`, { encoding: 'utf-8' }) | ||
| return { success: true, output: output.trim() } | ||
| } catch (e) { | ||
| return { success: false, output: e.stderr ? e.stderr.trim() : e.stdout ? e.stdout.trim() : e.message } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify shell-based child_process usage and missing timeout/maxBuffer in this test file.
rg -nP --type=js -C2 '\bexecSync\s*\(' tests/api/collectorconfig-webhook.test.js
rg -nP --type=js -C2 'execSync\s*\(\s*`' tests/api/collectorconfig-webhook.test.js
rg -nP --type=js -C2 'exec(File)?Sync\s*\([^)]*\{[^}]*encoding:\s*[\'"]utf-8[\'"][^}]*\}' tests/api/collectorconfig-webhook.test.jsRepository: stolostron/search-e2e-test
Length of output: 1837
🏁 Script executed:
#!/bin/bash
# Read the full test file to understand context and verify the issue
head -100 tests/api/collectorconfig-webhook.test.jsRepository: stolostron/search-e2e-test
Length of output: 2770
🏁 Script executed:
#!/bin/bash
# Check for Jest configuration and complete test file
find . -name "jest.config.*" -o -name "jest.setup.*" | head -5
echo "---"
wc -l tests/api/collectorconfig-webhook.test.js
echo "---"
tail -100 tests/api/collectorconfig-webhook.test.jsRepository: stolostron/search-e2e-test
Length of output: 2626
🏁 Script executed:
#!/bin/bash
# Check Jest config for timeout settings
cat jest.config.jsRepository: stolostron/search-e2e-test
Length of output: 513
Add timeouts and use execFileSync to harden oc command execution
Lines 23, 32, 42, and 54 run oc commands via execSync without timeout or maxBuffer settings. A hung oc command (network delay, slow cluster) or oversized webhook error output can stall CI indefinitely. Line 23 additionally uses shell string interpolation (echo '${yaml}'), which is fragile for edge cases with quotes or special characters.
Use execFileSync with input, timeout, and maxBuffer for robustness. For cleanup commands (lines 32, 42, 54), add at least timeout (30000ms) and maxBuffer (1MB).
Suggested patch
-const { execSync } = require('child_process')
+const { execSync, execFileSync } = require('child_process')
...
function applyCollectorConfig(spec) {
const yaml = `
apiVersion: search.open-cluster-management.io/v1alpha1
kind: CollectorConfig
metadata:
name: webhook-test
namespace: ${testNamespace}
spec:
${spec}`
try {
- const output = execSync(`echo '${yaml}' | oc apply -f - 2>&1`, { encoding: 'utf-8' })
+ const output = execFileSync('oc', ['apply', '-f', '-'], {
+ input: yaml,
+ encoding: 'utf-8',
+ timeout: 60000,
+ maxBuffer: 1024 * 1024,
+ stdio: ['pipe', 'pipe', 'pipe'],
+ })
return { success: true, output: output.trim() }
} catch (e) {
return { success: false, output: e.stderr ? e.stderr.trim() : e.stdout ? e.stdout.trim() : e.message }
}
}
...
- execSync(`oc delete collectorconfig webhook-test -n ${testNamespace} --ignore-not-found=true 2>&1`, {
- encoding: 'utf-8',
- })
+ execSync(`oc delete collectorconfig webhook-test -n ${testNamespace} --ignore-not-found=true 2>&1`, {
+ encoding: 'utf-8',
+ timeout: 30000,
+ maxBuffer: 1024 * 1024,
+ })
...
- execSync(`oc create namespace ${testNamespace} --dry-run=client -o yaml | oc apply -f - 2>&1`, {
- encoding: 'utf-8',
- })
+ execSync(`oc create namespace ${testNamespace} --dry-run=client -o yaml | oc apply -f - 2>&1`, {
+ encoding: 'utf-8',
+ timeout: 30000,
+ maxBuffer: 1024 * 1024,
+ })
...
- execSync(`oc delete namespace ${testNamespace} --ignore-not-found=true 2>&1`, { encoding: 'utf-8' })
+ execSync(`oc delete namespace ${testNamespace} --ignore-not-found=true 2>&1`, {
+ encoding: 'utf-8',
+ timeout: 30000,
+ maxBuffer: 1024 * 1024,
+ })🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 23-23: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 `@tests/api/collectorconfig-webhook.test.js` around lines 22 - 27, Replace the
fragile execSync shell calls that run `oc` with execFileSync and add timeouts
and maxBuffer to prevent CI hangs: for the `echo '${yaml}' | oc apply -f -`
invocation use execFileSync('oc', ['apply','-f','-'], { input: yaml, encoding:
'utf-8', timeout: 30000, maxBuffer: 1024*1024 }) instead of execSync so we pass
YAML via the input option (avoiding shell interpolation); for the cleanup
execSync calls that invoke `oc` (the other occurrences) replace them with
execFileSync and include at least timeout: 30000 and maxBuffer: 1024*1024 in
their options; preserve the existing catch/result logic but continue to read
e.stderr/e.stdout/e.message as before.
|
@smcavey: 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. |
1 participant
Testing some collectionRules spec with the CollectorConfig validating webhook. AWAITING 5.0 BUILD
Summary by CodeRabbit