hack/run_test.sh (2)

3-3: Replace non-ASCII characters in comments.

The comment contains Russian text which might cause encoding issues in some environments.

-#  run_test.sh — POSIX-sh test-runner с live-trace и корректным RC-выходом    #
+#  run_test.sh - POSIX-sh test-runner with live-trace and correct RC-exit     #

---

26-26: Translate Russian comments to English for better compatibility.

Several inline comments contain Russian text which might cause encoding issues.

-  skip_next="+ $fn"      # первую строку трассировки с именем функции пропустим
+  skip_next="+ $fn"      # skip first trace line with function name

           '__RC__'*) : ;;                        # маркер не выводим
+          '__RC__'*) : ;;                        # don't output marker

Also applies to: 37-42

hack/e2e.bats (1)

27-30: Consider checking file existence before reading.

While the current approach works with || true, it would be cleaner to check if PID files exist first.

 @test "Clean previous VMs" {
- kill $(cat srv1/qemu.pid srv2/qemu.pid srv3/qemu.pid 2>/dev/null) 2>/dev/null || true
+ for i in 1 2 3; do
+   [ -f "srv${i}/qemu.pid" ] && kill "$(cat "srv${i}/qemu.pid")" 2>/dev/null || true
+ done
  rm -rf srv1 srv2 srv3
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

hack/e2e-apps.bats (2)

3-3: Replace non-ASCII hyphen with standard ASCII hyphen.

The comment contains a non-ASCII hyphen character (‑) which could cause issues in some environments.

-# Cozystack end‑to‑end provisioning test (Bats)
+# Cozystack end-to-end provisioning test (Bats)

---

87-87: Remove redundant namespace status check.

The namespace tenant-test was already verified to be Active in the previous test. This check is redundant unless tests can be run independently.

If tests are meant to be run independently, consider adding a comment explaining this. Otherwise, remove this line:

-  kubectl wait namespace tenant-test --timeout=20s --for=jsonpath='{.status.phase}'=Active

hack/e2e-cluster.bats (4)

3-3: Replace non-ASCII hyphen with standard ASCII hyphen.

The comment contains a non-ASCII hyphen character (‑) which could cause issues in some environments.

-# Cozystack end‑to‑end provisioning test (Bats)
+# Cozystack end-to-end provisioning test (Bats)

---

59-59: Consider using consistent subnet mask.

The VMs are configured with /26 subnet mask while the bridge uses /24. This inconsistency might cause confusion.

For a 3-node test setup, using /24 throughout would be simpler and more consistent:

-      - "192.168.123.1${i}/26"
+      - "192.168.123.1${i}/24"

---

110-112: Consider more robust VM startup verification.

The fixed 5-second sleep might not be sufficient in all environments (slow systems, high load, etc.).

Since the next test already waits for the Talos API port, consider removing the sleep or replacing it with a network readiness check:

-  # Give qemu a few seconds to start up networking
-  sleep 5
+  # Network readiness will be verified in the next test

---

266-267: Improve readability by splitting the long kubectl wait command.

The command line is very long and hard to read.

Consider using a loop or splitting into multiple wait commands:

# Wait for all deployments
deployments=(
  "capi-controller-manager"
  "capi-kamaji-controller-manager"
  "capi-kubeadm-bootstrap-controller-manager"
  "capi-operator-cluster-api-operator"
  "capk-controller-manager"
)

for deploy in "${deployments[@]}"; do
  kubectl wait "deployment/${deploy}" -n cozy-cluster-api --timeout=1m --for=condition=available
done

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

hack/e2e-cluster.bats (3)

75-76: Progress output redirection may cause issues.

The combination of --output-file /dev/stdout and 2>/dev/null in the wget command could suppress important error messages while trying to redirect progress output.

Consider simplifying the progress handling:

-wget https://github.com/cozystack/cozystack/releases/latest/download/nocloud-amd64.raw.xz \
-  -O nocloud-amd64.raw.xz --show-progress --output-file /dev/stdout --progress=dot:giga 2>/dev/null
+wget https://github.com/cozystack/cozystack/releases/latest/download/nocloud-amd64.raw.xz \
+  -O nocloud-amd64.raw.xz --progress=dot:giga

---

101-107: Consider making VM resource allocation configurable.

The VM resources (8 CPUs, 16GB RAM) are hardcoded, which may not be suitable for all testing environments.

Consider using environment variables for resource configuration:

+# Set default values if not provided
+: ${QEMU_CPUS:=8}
+: ${QEMU_MEMORY:=16384}
+
qemu-system-x86_64 -machine type=pc,accel=kvm -cpu host -smp ${QEMU_CPUS} -m ${QEMU_MEMORY} \

---

115-115: Enhance timeout error message for better debugging.

The current timeout command doesn't provide clear information about which specific node failed to respond.

Consider adding more descriptive error handling:

-timeout 60 sh -ec 'until nc -nz 192.168.123.11 50000 && nc -nz 192.168.123.12 50000 && nc -nz 192.168.123.13 50000; do sleep 1; done'
+timeout 60 sh -ec '
+  until nc -nz 192.168.123.11 50000 && nc -nz 192.168.123.12 50000 && nc -nz 192.168.123.13 50000; do 
+    echo "Waiting for Talos API on nodes 11, 12, 13..." >&2
+    sleep 1
+  done'

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

hack/e2e-cluster.bats (2)

350-353: Force reconciliation still present from previous review

The forced reconciliation pattern was flagged in a previous review as potentially masking underlying issues.

As noted in the previous review, investigate why the monitoring HelmRelease occasionally requires force reconciliation instead of relying on this workaround.

---

27-27: ⚠️ Potential issue

Address unsafe process termination from previous review

This issue was flagged in a previous review but remains unresolved. The kill command could terminate unrelated processes if PIDs are stale.

Apply the suggested fix from the previous review:

-kill $(cat srv1/qemu.pid srv2/qemu.pid srv3/qemu.pid 2>/dev/null) 2>/dev/null || true
+for pidfile in srv1/qemu.pid srv2/qemu.pid srv3/qemu.pid; do
+  if [ -f "$pidfile" ]; then
+    pid=$(cat "$pidfile" 2>/dev/null || echo "")
+    if [ -n "$pid" ] && kill -0 "$pid" 2>/dev/null; then
+      kill "$pid" 2>/dev/null || true
+    fi
+  fi
+done

hack/cozytest.sh (4)

22-23: Improve error handling for temporary directory creation

The error handling is good, but the cleanup should be more robust.

 run_one() {
   fn=$1 title=$2
-  tmp=$(mktemp -d) || { echo "Failed to create temp directory" >&2; exit 1; }
+  tmp=$(mktemp -d) || { echo "Failed to create temp directory" >&2; exit 1; }
+  trap 'rm -rf "$tmp"' EXIT
   log="$tmp/log"

---

27-27: Remove non-English comment

The comment contains non-English text which should be translated for consistency.

-  skip_next="+ $fn"      # первую строку трассировки с именем функции пропустим
+  skip_next="+ $fn"      # skip the first trace line with function name

---

86-86: Remove non-English comment

Another non-English comment that should be translated.

-    print "  set -e"           # ошибка → падение теста
+    print "  set -e"           # error causes test failure

---

90-90: Remove non-English comment

Another non-English comment that should be translated.

-    print "  return 0"         # если автор не сделал exit 1 — тест ОК
+    print "  return 0"         # if author didn't exit 1, test is OK

hack/e2e-cluster.bats (1)

233-262: Consider making timeouts configurable

The hard-coded timeouts throughout the script (60s, 15m, 2m, etc.) might not be suitable for all environments. Consider making these configurable via environment variables.

Example for the HelmRelease wait:

+ HELM_TIMEOUT=${HELM_TIMEOUT:-15m}
- kubectl get hr -A | awk 'NR>1 {print "kubectl wait --timeout=15m --for=condition=ready -n "$1" hr/"$2" &"} END {print "wait"}' | sh -ex
+ kubectl get hr -A | awk -v timeout="$HELM_TIMEOUT" 'NR>1 {print "kubectl wait --timeout="timeout" --for=condition=ready -n "$1" hr/"$2" &"} END {print "wait"}' | sh -ex

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro