Repository navigation
changed test 15 to be about readiness-probe and add new test for name confusion - #811
Conversation
WalkthroughAdds a new Kubernetes-based test fixture for pods with names containing “service,” and refactors the existing failed readiness probe test to use local manifests. Removes recorded kubectl outputs, introduces new namespaces/manifests, and updates test prompts/expectations to reference pods instead of services. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 7
🧹 Nitpick comments (1)
tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml (1)
14-14: Pin the image tag for reproducibility.Use a fixed BusyBox tag (e.g., 1.36.1) to avoid drift.
- image: busybox + image: busybox:1.36.1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/kubectl_describe.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/kubectl_find_resource.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/kubectl_logs.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/test_case.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/kubectl_find_resource.txt
- tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/kubectl_describe.txt
- tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/kubectl_logs.txt
🧰 Additional context used
📓 Path-based instructions (3)
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/test_case.yamltests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yamltests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yamltests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/test_case.yaml
**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
ALWAYS use Secrets for scripts, not inline manifests or ConfigMaps (prevents code visibility with kubectl describe)
Files:
tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/test_case.yamltests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yamltests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yamltests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/test_case.yaml
tests/**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
tests/**/*.yaml: Never use names that hint at the problem or expected behavior in resource names (e.g., avoid 'broken-pod', 'test-project-that-does-not-exist', 'crashloop-app'). Use neutral names that don't give away what the LLM should discover
Each test must use a dedicated namespace app- to prevent conflicts
All pod names must be unique across tests
Resource naming should be neutral, not hint at the problem
Use minimal resource footprints (e.g., reduce memory/CPU for Loki in tests)
Files:
tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/test_case.yamltests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yamltests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yamltests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/test_case.yaml
🧠 Learnings (1)
📚 Learning: 2025-08-10T06:02:54.308Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.308Z
Learning: Applies to tests/**/*.yaml : Never use names that hint at the problem or expected behavior in resource names (e.g., avoid 'broken-pod', 'test-project-that-does-not-exist', 'crashloop-app'). Use neutral names that don't give away what the LLM should discover
Applied to files:
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/test_case.yaml
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yaml
[MEDIUM] 6-24: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-24: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml
[MEDIUM] 6-24: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-24: Minimize the admission of root containers
(CKV_K8S_23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (2)
tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/test_case.yaml (1)
1-1: No changes needed: pod name remainssearch-engineThe test’s manifest explicitly names the Pod
search-engine(uniqueness is achieved via thetest-15namespace). The existinguser_promptandexpected_outputcorrectly referencesearch-engine, so you can ignore the suggestion to rename it tosearch-engine-15.Likely an incorrect or invalid review comment.
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/test_case.yaml (1)
1-1: Sanity-check prompt vs resource names.Prompt mentions search-engine-service but the manifest creates search-engine[-111]. If intentional (to test name confusion), fine; otherwise, update for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yaml (1)
12-24: Secretize scripts and harden the container (CKV_K8S_20, CKV_K8S_23).Move inline commands to a Secret, pin the image, add a restrictive securityContext, and minimal resources. This aligns with tests/**/*.yaml guidance and fixes Checkov findings.
spec: containers: - name: get-details - image: busybox - command: ["sh", "-c", "while true; do echo 'Running...'; sleep 5; done"] + image: busybox:1.36.1 + command: ["sh", "/scripts/loop.sh"] + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 1000 + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] + resources: + requests: + cpu: "5m" + memory: "16Mi" + limits: + cpu: "10m" + memory: "32Mi" readinessProbe: - exec: - command: - - sh - - -c - - "exit 1" + exec: + command: ["sh", "/scripts/always_fail.sh"] initialDelaySeconds: 5 periodSeconds: 5 failureThreshold: 3 + volumeMounts: + - name: scripts + mountPath: /scripts + readOnly: true + volumes: + - name: scripts + secret: + secretName: app-111-scriptsAdd the Secret (outside this hunk):
apiVersion: v1 kind: Secret metadata: name: app-111-scripts namespace: app-111 type: Opaque stringData: loop.sh: | #!/bin/sh while true; do echo 'Running...' sleep 5 done always_fail.sh: | #!/bin/sh exit 1tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml (2)
9-9: Ensure pod name is unique across tests.Append the test id to guarantee uniqueness across the suite.
- name: search-engine + name: search-engine-15To detect any remaining duplicates across the repo, run:
#!/bin/bash # List Pod names and flag duplicates across tests rg -n --no-heading $'^kind:\\s*Pod\\s*$' -A 5 \ | gawk ' /kind:\s*Pod/ {file=$0} /name:\s*/ && prev_kind { # extract filename from previous line containing the path (rg prints "path:line") # we’ll just print the path line for context and the name } { print } ' | sed -n 's/.*name: *//p' | sort | uniq -d
12-24: Secretize scripts and harden the container (CKV_K8S_20, CKV_K8S_23).Move inline commands to a Secret, pin image, add restrictive securityContext, and minimal resources to reduce footprint.
spec: containers: - name: get-details - image: busybox - command: ["sh", "-c", "while true; do echo 'Running...'; sleep 5; done"] + image: busybox:1.36.1 + command: ["sh", "/scripts/loop.sh"] + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 1000 + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] + resources: + requests: + cpu: "5m" + memory: "16Mi" + limits: + cpu: "10m" + memory: "32Mi" readinessProbe: - exec: - command: - - sh - - -c - - "exit 1" + exec: + command: ["sh", "/scripts/always_fail.sh"] initialDelaySeconds: 5 periodSeconds: 5 failureThreshold: 3 + volumeMounts: + - name: scripts + mountPath: /scripts + readOnly: true + volumes: + - name: scripts + secret: + secretName: app-15-scriptsAdd the Secret (outside this hunk):
apiVersion: v1 kind: Secret metadata: name: app-15-scripts namespace: app-15 type: Opaque stringData: loop.sh: | #!/bin/sh while true; do echo 'Running...' sleep 5 done always_fail.sh: | #!/bin/sh exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/test_case.yaml
🧰 Additional context used
📓 Path-based instructions (3)
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yamltests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml
**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
ALWAYS use Secrets for scripts, not inline manifests or ConfigMaps (prevents code visibility with kubectl describe)
Files:
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yamltests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml
tests/**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
tests/**/*.yaml: Never use names that hint at the problem or expected behavior in resource names (e.g., avoid 'broken-pod', 'test-project-that-does-not-exist', 'crashloop-app'). Use neutral names that don't give away what the LLM should discover
Each test must use a dedicated namespace app- to prevent conflicts
All pod names must be unique across tests
Resource naming should be neutral, not hint at the problem
Use minimal resource footprints (e.g., reduce memory/CPU for Loki in tests)
Files:
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yamltests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.308Z
Learning: Applies to tests/**/*.yaml : All pod names must be unique across tests
📚 Learning: 2025-08-10T06:02:54.308Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.308Z
Learning: Applies to tests/**/*.yaml : Each test must use a dedicated namespace app-<testid> to prevent conflicts
Applied to files:
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yamltests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml
📚 Learning: 2025-08-10T06:02:54.308Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.308Z
Learning: Applies to tests/**/*.yaml : Never use names that hint at the problem or expected behavior in resource names (e.g., avoid 'broken-pod', 'test-project-that-does-not-exist', 'crashloop-app'). Use neutral names that don't give away what the LLM should discover
Applied to files:
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yamltests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml
📚 Learning: 2025-08-10T06:02:54.308Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.308Z
Learning: Applies to tests/**/*.yaml : All pod names must be unique across tests
Applied to files:
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yamltests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml
📚 Learning: 2025-08-10T06:02:54.308Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.308Z
Learning: Applies to **/*.yaml : ALWAYS use Secrets for scripts, not inline manifests or ConfigMaps (prevents code visibility with kubectl describe)
Applied to files:
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yamltests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yaml
[MEDIUM] 6-24: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-24: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml
[MEDIUM] 6-24: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-24: Minimize the admission of root containers
(CKV_K8S_23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
🔇 Additional comments (2)
tests/llm/fixtures/test_ask_holmes/111_pod_names_contain_service/manifest.yaml (1)
1-5: Namespace convention and scoping look good.Namespace uses app-111 and is correctly declared.
tests/llm/fixtures/test_ask_holmes/15_failed_readiness_probe/manifest.yaml (1)
1-5: Namespace convention is correct.Using app-15 matches the guideline.
2 participants
No description provided.