[Tests] Add Kafka, Redis - #1077
Conversation
|
""" WalkthroughTwo new BATS test scripts have been added to automate end-to-end testing of Kafka and Redis custom resources in a Kubernetes environment. These scripts create the resources, verify their readiness and associated components, and perform cleanup by deleting the resources after the tests complete. Additionally, the GitHub Actions workflow was updated to include Kafka and Redis in the test matrix for pull request validation. The Postgres test script was also updated to delete an associated init job during cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Kubernetes
participant Kafka/Redis Operator
Tester->>Kubernetes: Create Kafka/Redis resource manifest
Kubernetes->>Kafka/Redis Operator: Notify of new resource
Kafka/Redis Operator->>Kubernetes: Create required components (PVC, StatefulSet, Service, etc.)
Tester->>Kubernetes: Poll for resource readiness and component status
Kubernetes->>Tester: Respond with status
Tester->>Kubernetes: Delete Kafka/Redis resource after test
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🪛 YAMLlint (1.37.1).github/workflows/pull-requests.yaml[warning] 158-158: too few spaces after comma (commas) [warning] 158-158: too few spaces after comma (commas) [warning] 158-158: too few spaces after comma (commas) [warning] 158-158: too few spaces after comma (commas) [warning] 158-158: too few spaces after comma (commas) [warning] 158-158: too few spaces after comma (commas) [warning] 158-158: too few spaces after comma (commas) ✨ Finishing Touches
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. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
hack/e2e-apps.bats (1)
404-407: Redis bootstrap may exceed 20 s/70 s – raise the ceilingsInitial PVC binding plus sentinel & redis-server rollout often crosses the current limits, causing flaky red builds. Suggest increasing to 2-3 minutes each to stabilise CI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/e2e-apps.bats(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pre-commit
- GitHub Check: Build
0178c34 to
ebae746
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
hack/e2e-apps/kafka.bats (1)
24-24: Clarify empty resources field.The
resources:field is empty, which might be intentional but could be confusing.Consider making it explicit:
- resources: + resources: {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hack/e2e-apps/kafka.bats(1 hunks)hack/e2e-apps/redis.bats(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pre-commit
- GitHub Check: Build
| @test "Create Kafka" { | ||
| name='test' | ||
| kubectl create -f- <<EOF | ||
| apiVersion: apps.cozystack.io/v1alpha1 | ||
| kind: Kafka | ||
| metadata: | ||
| name: $name | ||
| namespace: tenant-test | ||
| spec: | ||
| external: false | ||
| kafka: | ||
| size: 10Gi | ||
| replicas: 2 | ||
| storageClass: "" | ||
| resources: {} | ||
| resourcesPreset: "nano" | ||
| zookeeper: | ||
| size: 5Gi | ||
| replicas: 2 | ||
| storageClass: "" | ||
| resources: | ||
| resourcesPreset: "nano" | ||
| topics: | ||
| - name: testResults | ||
| partitions: 1 | ||
| replicas: 2 | ||
| config: | ||
| min.insync.replicas: 2 | ||
| - name: testOrders | ||
| config: | ||
| cleanup.policy: compact | ||
| segment.ms: 3600000 | ||
| max.compaction.lag.ms: 5400000 | ||
| min.insync.replicas: 2 | ||
| partitions: 1 | ||
| replicas: 2 | ||
| EOF | ||
| sleep 5 | ||
| kubectl -n tenant-test wait hr kafka-$name --timeout=30s --for=condition=ready | ||
| kubectl -n tenant-test wait pvc data-kafka-$name-zookeeper-0 --timeout=30s --for=jsonpath='{.status.phase}'=Bound | ||
| timeout 40 sh -ec "until kubectl -n tenant-test get svc kafka-$name-zookeeper-client -o jsonpath='{.spec.ports[0].port}' | grep -q '2181'; do sleep 10; done" | ||
| timeout 40 sh -ec "until kubectl -n tenant-test get svc kafka-$name-zookeeper-nodes -o jsonpath='{.spec.ports[*].port}' | grep -q '2181 2888 3888'; do sleep 10; done" | ||
| timeout 80 sh -ec "until kubectl -n tenant-test get endpoints kafka-$name-zookeeper-nodes -o jsonpath='{.subsets[*].addresses[0].ip}' | grep -q '[0-9]'; do sleep 10; done" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup and consistent error handling.
Like the Redis test, this needs proper cleanup and consistent error handling throughout.
Add cleanup trap at the beginning of the test:
@test "Create Kafka" {
name="test-$(date +%s)-$$"
+
+ # Cleanup function
+ cleanup() {
+ kubectl delete kafka $name -n tenant-test --ignore-not-found=true || true
+ }
+ trap cleanup EXIT
+
kubectl create -f- <<EOFCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In hack/e2e-apps/kafka.bats around lines 3 to 46, the Kafka creation test lacks
cleanup and consistent error handling. Add a cleanup trap at the start of the
test to delete the Kafka resource and related PVCs after the test finishes or
fails. Also, ensure all kubectl commands and waits handle errors consistently,
for example by checking command success and failing the test early if any step
fails.
24d3d95 to
d5f6983
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pull-requests.yaml (1)
158-158: Insert spaces after commas in the app matrix list for YAML lint complianceYAML-lint is already flagging the line; adding a single space after each comma restores conventional YAML style and silences the warning.
- app: [clickhouse,kafka,kubernetes,mysql,postgres,redis,virtualmachine,vminstance] + app: [clickhouse, kafka, kubernetes, mysql, postgres, redis, virtualmachine, vminstance]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pull-requests.yaml(1 hunks)hack/e2e-apps/kafka.bats(1 hunks)hack/e2e-apps/redis.bats(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/e2e-apps/redis.bats
- hack/e2e-apps/kafka.bats
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pull-requests.yaml
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
🔇 Additional comments (1)
.github/workflows/pull-requests.yaml (1)
155-175: ```shell
#!/bin/bashCheck for a generic catch-all rule for test-apps in the core testing Makefile
grep -nH -E 'test-apps-%:' packages/core/testing/Makefile || echo "No explicit pattern rule 'test-apps-%:' found"
grep -nH -E '.PHONY:.*test-apps-%' packages/core/testing/Makefile || echo "No .PHONY entry for 'test-apps-%' found"</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/apps/clickhouse/values.yaml (1)
60-62: Default preset bumped to “small” – verify cluster quotas & update docsRaising the default
resourcesPresetfrom"nano"to"small"will increase CPU/memory requests for every deployment that does not override this value. Before merging, please confirm:• All CI/demo/production clusters have sufficient quota.
• CHANGELOG / user docs highlight the new default.
• Workloads depending on minimal footprints (e.g., local dev) explicitly override back to"nano"if needed.Quick scan to locate charts/manifests that rely on the implicit default:
#!/bin/bash # Finds ClickHouse releases with no explicit resourcesPreset override rg -n --no-heading $'resourcesPreset:' | grep -i clickhouse | grep -v '"nano"' | grep -v '"small"'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
hack/e2e-apps/clickhouse.bats(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/values.yaml(1 hunks)packages/apps/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/apps/clickhouse/Chart.yaml
- hack/e2e-apps/clickhouse.bats
- packages/apps/versions_map
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
d4c49b8 to
4b1051c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pull-requests.yaml (1)
158-158: Style: add spaces after commas to silence YAMLlint & improve readabilityYAMLlint flags the current line for “too few spaces after comma.”
Although syntactically valid, keeping consistent spacing avoids CI style warnings and makes the matrix easier to scan.- app: [clickhouse,kafka,kubernetes,mysql,postgres,redis,virtualmachine,vminstance] + app: [clickhouse, kafka, kubernetes, mysql, postgres, redis, virtualmachine, vminstance]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pull-requests.yaml(1 hunks)hack/e2e-apps/kafka.bats(1 hunks)hack/e2e-apps/redis.bats(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/e2e-apps/redis.bats
- hack/e2e-apps/kafka.bats
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pull-requests.yaml
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (1)
.github/workflows/pull-requests.yaml (1)
171-175: Verify Makefile targets for the newly-addedkafka&redisappsEach matrix job ultimately runs:
make -C packages/core/testing SANDBOX_NAME=$SANDBOX_NAME test-apps-${{ matrix.app }}If
test-apps-kafkaortest-apps-redistargets are missing (or mis-named) the job will fail at runtime.#!/bin/bash # Confirm that the expected Makefile targets exist. fd -I Makefile | xargs grep -nE 'test-apps-(kafka|redis)' || { echo "::error ::Missing Makefile targets: test-apps-kafka / test-apps-redis" exit 1 }
4b1051c to
2fec259
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/pull-requests.yaml(1 hunks)hack/e2e-apps/kafka.bats(1 hunks)hack/e2e-apps/postgres.bats(1 hunks)hack/e2e-apps/redis.bats(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/e2e-apps/redis.bats
- hack/e2e-apps/kafka.bats
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pull-requests.yaml
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
🔇 Additional comments (1)
hack/e2e-apps/postgres.bats (1)
53-53: LGTM! Good cleanup improvement.The addition of explicit init job deletion ensures proper cleanup of all resources created during the PostgreSQL test, preventing resource leakage in the test environment.
| strategy: | ||
| matrix: | ||
| app: [clickhouse,kubernetes,mysql,postgres,virtualmachine,vminstance] | ||
| app: [clickhouse,kafka,kubernetes,mysql,postgres,redis,virtualmachine,vminstance] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Verification correct
Fix YAML formatting and verify test files exist.
The addition of kafka and redis to the test matrix aligns with the PR objectives. However, there are YAML formatting issues that need to be addressed.
Apply this diff to fix the comma spacing:
- app: [clickhouse,kafka,kubernetes,mysql,postgres,redis,virtualmachine,vminstance]
+ app: [clickhouse, kafka, kubernetes, mysql, postgres, redis, virtualmachine, vminstance]Please verify that the corresponding test files exist:
🏁 Script executed:
#!/bin/bash
# Description: Verify that kafka.bats and redis.bats test files exist
# Expected: Both files should exist in the hack/e2e-apps/ directory
echo "Checking for Kafka and Redis test files..."
fd "kafka.bats" hack/e2e-apps/
fd "redis.bats" hack/e2e-apps/
echo "Listing all .bats files in hack/e2e-apps/ for context:"
fd "*.bats" hack/e2e-apps/Length of output: 640
Fix YAML formatting and verify test files exist.
The addition of kafka and redis to the test matrix aligns with the PR objectives. However, there are YAML formatting issues that need to be addressed.
Apply this diff to fix the comma spacing:
- app: [clickhouse,kafka,kubernetes,mysql,postgres,redis,virtualmachine,vminstance]
+ app: [clickhouse, kafka, kubernetes, mysql, postgres, redis, virtualmachine, vminstance]Please verify that the corresponding test files exist:
#!/bin/bash
# Description: Verify that kafka.bats and redis.bats test files exist
# Expected: Both files should exist in the hack/e2e-apps/ directory
echo "Checking for Kafka and Redis test files..."
fd "kafka.bats" hack/e2e-apps/
fd "redis.bats" hack/e2e-apps/
echo "Listing all .bats files in hack/e2e-apps/ for context:"
fd "*.bats" hack/e2e-apps/🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
[warning] 158-158: too few spaces after comma
(commas)
🤖 Prompt for AI Agents
In .github/workflows/pull-requests.yaml at line 158, fix the YAML formatting by
ensuring proper spacing after commas in the app list to comply with YAML syntax.
Additionally, verify that the test files kafka.bats and redis.bats exist in the
hack/e2e-apps/ directory by running the provided shell commands or equivalent
checks, and confirm their presence before finalizing the changes.
2fec259 to
4d09c86
Compare
4d09c86 to
082c40b
Compare
74d4502 to
d948980
Compare
Remove postgres job after completion Signed-off-by: Ahmad Murzahmatov <gwynbleidd2106@yandex.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
d948980 to
c0b7f4e
Compare
Add extra tests into e2e apps
Summary by CodeRabbit