Run E2E tests as separate parallel jobs - #1093
Conversation
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
WalkthroughThis update introduces new BATS end-to-end test scripts for various resources (databases, virtual machines, Kubernetes control planes, tenants) and refactors the CI workflow. The workflow now centralizes workspace usage in Changes
Sequence Diagram(s)sequenceDiagram
participant CI
participant PrepareEnv
participant InstallCozystack
participant SetupTenant
participant TestApp as TestApps (matrix)
participant Cleanup
CI->>PrepareEnv: Start job (move workspace, set timer)
PrepareEnv->>InstallCozystack: Trigger after prepare
InstallCozystack->>SetupTenant: Trigger after install
SetupTenant->>TestApp: Trigger after tenant setup
loop For each app
TestApp->>TestApp: Run app-specific test in /tmp/$SANDBOX_NAME
end
TestApp->>Cleanup: On completion, trigger cleanup
Cleanup->>CI: Remove sandbox
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (6)
hack/e2e-apps/mysql.bats (2)
5-5: Add error handling for kubectl get command.The kubectl get command should handle the case where the resource doesn't exist more gracefully.
- kubectl -n tenant-test get mysqls.apps.cozystack.io $name || + kubectl -n tenant-test get mysqls.apps.cozystack.io $name 2>/dev/null ||
40-45: Consider adjusting timeout values for better reliability.The timeout values for service and endpoint checks might need adjustment based on actual deployment times. Consider making these configurable or adding retry logic.
- timeout 80 sh -ec "until kubectl -n tenant-test get svc mysql-$name -o jsonpath='{.spec.ports[0].port}' | grep -q '3306'; do sleep 10; done" - timeout 80 sh -ec "until kubectl -n tenant-test get endpoints mysql-$name -o jsonpath='{.subsets[*].addresses[*].ip}' | grep -q '[0-9]'; do sleep 10; done" + timeout 120 sh -ec "until kubectl -n tenant-test get svc mysql-$name -o jsonpath='{.spec.ports[0].port}' | grep -q '3306'; do sleep 5; done" + timeout 120 sh -ec "until kubectl -n tenant-test get endpoints mysql-$name -o jsonpath='{.subsets[*].addresses[*].ip}' | grep -q '[0-9]'; do sleep 5; done"hack/e2e-apps/kubernetes.bats (2)
4-4: Add error handling for kubectl get command.Similar to other test files, the kubectl get command should handle the case where the resource doesn't exist more gracefully.
- kubectl -n tenant-test get kuberneteses.apps.cozystack.io test || + kubectl -n tenant-test get kuberneteses.apps.cozystack.io test 2>/dev/null ||
67-71: Consider timeout consistency across wait commands.The timeout values vary significantly (20s, 10s, 4m, 2m, 4m, 1m, 10m). Consider standardizing timeout patterns or documenting the rationale for different values.
For better maintainability, consider defining timeout constants at the top of the file:
+# Timeout constants +NAMESPACE_TIMEOUT=30s +CONTROL_PLANE_TIMEOUT=5m +DEPLOYMENT_TIMEOUT=5m +MACHINE_TIMEOUT=12mhack/e2e-apps/vminstance.bats (1)
50-61: Security concern: Hardcoded SSH keys in test configuration.The test contains hardcoded SSH keys both in the
sshKeysfield and in the cloud-init configuration. Consider using generated or environment-provided keys for better security practices.sshKeys: - - ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPht0dPk5qQ+54g1hSX7A6AUxXJW5T6n/3d7Ga2F8gTF - test@test + - ${TEST_SSH_KEY:-ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPht0dPk5qQ+54g1hSX7A6AUxXJW5T6n/3d7Ga2F8gTF test@test}.github/workflows/pull-requests.yaml (1)
151-151: Fix comma spacing in matrix array.The matrix array is missing spaces after commas as flagged by the linter.
- app: [clickhouse,kubernetes,mysql,postgres,virtualmachine,vminstance] + app: [clickhouse, kubernetes, mysql, postgres, virtualmachine, vminstance]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/pull-requests.yaml(4 hunks)hack/e2e-apps/clickhouse.bats(1 hunks)hack/e2e-apps/kubernetes.bats(1 hunks)hack/e2e-apps/mysql.bats(1 hunks)hack/e2e-apps/postgres.bats(1 hunks)hack/e2e-apps/tenant.bats(1 hunks)hack/e2e-apps/virtualmachine.bats(1 hunks)hack/e2e-apps/vminstance.bats(1 hunks)packages/core/testing/Makefile(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pull-requests.yaml
[error] 99-99: trailing spaces
(trailing-spaces)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
🔇 Additional comments (4)
hack/e2e-apps/tenant.bats (1)
3-22: Verify if cleanup is intentionally omitted.Unlike other tests, this one doesn't clean up the created tenant resource. Is this intentional because other tests depend on this tenant?
#!/bin/bash # Check if other test files reference the tenant-test namespace or test tenant echo "Checking for references to tenant-test namespace in other test files:" fd '\.bats$' hack/e2e-apps/ --exec grep -l "tenant-test" {} echo -e "\nChecking for tenant cleanup in other files:" rg -A 3 -B 3 "delete.*tenant" hack/e2e-apps/hack/e2e-apps/kubernetes.bats (1)
3-73: Well-structured e2e test with comprehensive resource validation.The test properly validates the complete lifecycle of a Kubernetes control plane including namespace readiness, control plane creation, resource validation, and cleanup. The configuration covers all necessary components for a functional tenant cluster.
.github/workflows/pull-requests.yaml (2)
129-146: Well-designed job separation for improved CI parallelization.The separation of tenant setup from individual app tests enables better parallelization and clearer job dependencies. This is a good architectural improvement for the CI pipeline.
148-167: Excellent matrix strategy for parallel test execution.The matrix-based approach for running individual app tests allows for better parallelization and easier identification of which specific tests are failing. This significantly improves the CI experience.
| cpu: "" | ||
| memory: "" | ||
| sshKeys: | ||
| - ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPht0dPk5qQ+54g1hSX7A6AUxXJW5T6n/3d7Ga2F8gTF |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded SSH keys with test-specific keys.
The SSH keys appear to be hardcoded across multiple lines. For security and maintainability, consider using test-specific keys or environment variables.
- - ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPht0dPk5qQ+54g1hSX7A6AUxXJW5T6n/3d7Ga2F8gTF
- test@test
+ - ${TEST_SSH_KEY:-ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPht0dPk5qQ+54g1hSX7A6AUxXJW5T6n/3d7Ga2F8gTF test@test}Apply similar changes to line 38 in the cloud-init section.
Also applies to: 38-38
🤖 Prompt for AI Agents
In hack/e2e-apps/virtualmachine.bats at lines 28 and 38, replace the hardcoded
SSH keys with test-specific keys or load them from environment variables to
improve security and maintainability. Remove the static key strings and modify
the script to dynamically inject or reference keys appropriate for the test
environment instead.
| kubectl -n tenant-test wait pvc virtual-machine-$name --timeout=100s --for=jsonpath='{.status.phase}'=Bound | ||
| kubectl -n tenant-test wait vm virtual-machine-$name --timeout=100s --for=condition=ready | ||
| timeout 120 sh -ec "until kubectl -n tenant-test get vmi virtual-machine-$name -o jsonpath='{.status.interfaces[0].ipAddress}' | grep -q '[0-9]'; do sleep 10; done" | ||
| kubectl -n tenant-test delete virtualmachines.apps.cozystack.io $name |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure cleanup happens even if test fails.
The cleanup only runs if all previous commands succeed. Consider using a trap or teardown function to ensure cleanup occurs regardless of test outcome.
Add at the beginning of the test:
+ # Ensure cleanup on test failure
+ trap 'kubectl -n tenant-test delete virtualmachines.apps.cozystack.io $name 2>/dev/null || true' EXIT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubectl -n tenant-test delete virtualmachines.apps.cozystack.io $name | |
| # Ensure cleanup on test failure | |
| trap 'kubectl -n tenant-test delete virtualmachines.apps.cozystack.io $name 2>/dev/null || true' EXIT | |
| kubectl -n tenant-test delete virtualmachines.apps.cozystack.io $name |
🤖 Prompt for AI Agents
In hack/e2e-apps/virtualmachine.bats at line 47, the cleanup command deleting
the virtual machine only runs if prior commands succeed, risking leftover
resources if the test fails. To fix this, add a trap or teardown function at the
start of the test that ensures the kubectl delete command runs regardless of
test success or failure, guaranteeing cleanup always occurs.
|
|
||
| @test "Create a Virtual Machine" { | ||
| name='test' | ||
| kubectl -n tenant-test get virtualmachines.apps.cozystack.io $name || |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for resource existence check.
The || operator will proceed with resource creation even if the kubectl get command fails for reasons other than resource not existing (e.g., network issues, permission errors).
- kubectl -n tenant-test get virtualmachines.apps.cozystack.io $name ||
+ if ! kubectl -n tenant-test get virtualmachines.apps.cozystack.io $name >/dev/null 2>&1; thenAnd close the conditional after the EOF:
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubectl -n tenant-test get virtualmachines.apps.cozystack.io $name || | |
| # Only create the VM if it doesn’t already exist | |
| if ! kubectl -n tenant-test get virtualmachines.apps.cozystack.io $name >/dev/null 2>&1; then | |
| kubectl -n tenant-test apply -f - <<EOF | |
| apiVersion: apps.cozystack.io/v1alpha1 | |
| kind: VirtualMachine | |
| metadata: | |
| name: $name | |
| namespace: tenant-test | |
| spec: | |
| # …rest of your VM spec… | |
| EOF | |
| fi |
🤖 Prompt for AI Agents
In hack/e2e-apps/virtualmachine.bats at line 5, the use of `||` after the
`kubectl get` command can cause the script to proceed with resource creation
even if the failure is due to reasons other than the resource not existing. To
fix this, replace the `||` operator with explicit error handling that checks if
the failure is specifically due to the resource not being found. Also, ensure
the conditional block is properly closed after the EOF to maintain correct
script structure.
| maxSyncReplicas: 0 | ||
| users: | ||
| testuser: | ||
| password: xai7Wepo |
There was a problem hiding this comment.
Replace hardcoded credentials with environment variables.
Hardcoded passwords and keys pose security risks and reduce test flexibility.
- password: xai7Wepo
+ password: ${TEST_DB_PASSWORD:-xai7Wepo}Apply similar changes to backup credentials:
- s3AccessKey: oobaiRus9pah8PhohL1ThaeTa4UVa7gu
- s3SecretKey: ju3eum4dekeich9ahM1te8waeGai0oog
- resticPassword: ChaXoveekoh6eigh4siesheeda2quai0
+ s3AccessKey: ${TEST_S3_ACCESS_KEY:-oobaiRus9pah8PhohL1ThaeTa4UVa7gu}
+ s3SecretKey: ${TEST_S3_SECRET_KEY:-ju3eum4dekeich9ahM1te8waeGai0oog}
+ resticPassword: ${TEST_RESTIC_PASSWORD:-ChaXoveekoh6eigh4siesheeda2quai0}Also applies to: 37-39
🤖 Prompt for AI Agents
In hack/e2e-apps/postgres.bats at line 25 and also lines 37 to 39, replace the
hardcoded password and backup credentials with environment variables. Modify the
script to read these sensitive values from environment variables instead of
embedding them directly in the code, improving security and flexibility. Use
appropriate environment variable names and ensure the script references them
correctly.
|
|
||
| @test "Create DB ClickHouse" { | ||
| name='test' | ||
| kubectl -n tenant-test get clickhouses.apps.cozystack.io $name || |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for resource existence check.
Same error handling issue as other tests.
- kubectl -n tenant-test get clickhouses.apps.cozystack.io $name ||
+ if ! kubectl -n tenant-test get clickhouses.apps.cozystack.io $name >/dev/null 2>&1; thenAnd close the conditional after the EOF:
+ fiCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In hack/e2e-apps/clickhouse.bats at line 5, the command checking for the
existence of the Clickhouse resource lacks proper error handling and does not
close the conditional block after the EOF. Modify the script to include a
conditional statement that properly handles the failure case of the kubectl get
command, ensuring the conditional block is correctly closed after the EOF to
improve error handling consistency with other tests.
| testuser: | ||
| maxUserConnections: 1000 | ||
| password: xai7Wepo | ||
| databases: | ||
| testdb: | ||
| roles: | ||
| admin: | ||
| - testuser | ||
| backup: | ||
| enabled: false | ||
| s3Region: us-east-1 | ||
| s3Bucket: s3.example.org/postgres-backups | ||
| schedule: "0 2 * * *" | ||
| cleanupStrategy: "--keep-last=3 --keep-daily=3 --keep-within-weekly=1m" | ||
| s3AccessKey: oobaiRus9pah8PhohL1ThaeTa4UVa7gu | ||
| s3SecretKey: ju3eum4dekeich9ahM1te8waeGai0oog | ||
| resticPassword: ChaXoveekoh6eigh4siesheeda2quai0 |
There was a problem hiding this comment.
Security concern: Hardcoded credentials in test configuration.
The test contains hardcoded passwords, access keys, and secret keys. While this might be acceptable for e2e tests, consider using environment variables or generated values to improve security practices.
Also, the backup configuration references "postgres-backups" in the S3 bucket path, which seems incorrect for a MySQL test.
- password: xai7Wepo
+ password: ${MYSQL_TEST_PASSWORD:-xai7Wepo}- s3Bucket: s3.example.org/postgres-backups
+ s3Bucket: s3.example.org/mysql-backups- s3AccessKey: oobaiRus9pah8PhohL1ThaeTa4UVa7gu
- s3SecretKey: ju3eum4dekeich9ahM1te8waeGai0oog
- resticPassword: ChaXoveekoh6eigh4siesheeda2quai0
+ s3AccessKey: ${S3_ACCESS_KEY:-oobaiRus9pah8PhohL1ThaeTa4UVa7gu}
+ s3SecretKey: ${S3_SECRET_KEY:-ju3eum4dekeich9ahM1te8waeGai0oog}
+ resticPassword: ${RESTIC_PASSWORD:-ChaXoveekoh6eigh4siesheeda2quai0}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testuser: | |
| maxUserConnections: 1000 | |
| password: xai7Wepo | |
| databases: | |
| testdb: | |
| roles: | |
| admin: | |
| - testuser | |
| backup: | |
| enabled: false | |
| s3Region: us-east-1 | |
| s3Bucket: s3.example.org/postgres-backups | |
| schedule: "0 2 * * *" | |
| cleanupStrategy: "--keep-last=3 --keep-daily=3 --keep-within-weekly=1m" | |
| s3AccessKey: oobaiRus9pah8PhohL1ThaeTa4UVa7gu | |
| s3SecretKey: ju3eum4dekeich9ahM1te8waeGai0oog | |
| resticPassword: ChaXoveekoh6eigh4siesheeda2quai0 | |
| testuser: | |
| maxUserConnections: 1000 | |
| password: ${MYSQL_TEST_PASSWORD:-xai7Wepo} | |
| databases: | |
| testdb: | |
| roles: | |
| admin: | |
| - testuser | |
| backup: | |
| enabled: false | |
| s3Region: us-east-1 | |
| s3Bucket: s3.example.org/mysql-backups | |
| schedule: "0 2 * * *" | |
| cleanupStrategy: "--keep-last=3 --keep-daily=3 --keep-within-weekly=1m" | |
| s3AccessKey: ${S3_ACCESS_KEY:-oobaiRus9pah8PhohL1ThaeTa4UVa7gu} | |
| s3SecretKey: ${S3_SECRET_KEY:-ju3eum4dekeich9ahM1te8waeGai0oog} | |
| resticPassword: ${RESTIC_PASSWORD:-ChaXoveekoh6eigh4siesheeda2quai0} |
🤖 Prompt for AI Agents
In hack/e2e-apps/mysql.bats around lines 18 to 34, the test configuration
contains hardcoded sensitive credentials and an incorrect S3 bucket path
referencing "postgres-backups" instead of a MySQL-related bucket. Replace all
hardcoded passwords, access keys, and secret keys with environment variables or
dynamically generated values to enhance security. Also, update the S3 bucket
path to correctly reflect a MySQL backup location consistent with the test
context.
| kubectl -n tenant-test delete vminstances.apps.cozystack.io $name | ||
| kubectl -n tenant-test delete vmdisks.apps.cozystack.io $diskName | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential test dependency issue between VM Disk and VM Instance tests.
The VM Instance test deletes both the VMInstance and VMDisk, but it assumes the VMDisk exists from a previous test. This creates an implicit dependency between tests which can make them fragile.
Consider either:
- Making the VM Instance test create its own disk, or
- Combining both tests into a single comprehensive test, or
- Using BATS setup/teardown functions to manage shared resources
@test "Create a VM Instance" {
diskName='test'
name='test'
+ # Ensure the disk exists for this test
+ kubectl -n tenant-test get vmdisks.apps.cozystack.io $diskName || {
+ echo "VMDisk $diskName not found. Please run 'Create a VM Disk' test first."
+ return 1
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In hack/e2e-apps/vminstance.bats around lines 68 to 70, the VM Instance test
deletes a VMDisk that it assumes was created by a previous test, causing a
fragile dependency. To fix this, modify the VM Instance test to either create
its own VMDisk before running or combine the VMInstance and VMDisk tests into
one comprehensive test. Alternatively, implement BATS setup and teardown
functions to manage creation and cleanup of shared resources like VMDisks to
ensure tests are independent and reliable.
| @test "Create a VM Disk" { | ||
| name='test' | ||
| kubectl -n tenant-test get vmdisks.apps.cozystack.io $name || | ||
| kubectl create -f - <<EOF | ||
| apiVersion: apps.cozystack.io/v1alpha1 | ||
| kind: VMDisk | ||
| metadata: | ||
| name: $name | ||
| namespace: tenant-test | ||
| spec: | ||
| source: | ||
| http: | ||
| url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img | ||
| optical: false | ||
| storage: 5Gi | ||
| storageClass: replicated | ||
| EOF | ||
| sleep 5 | ||
| kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready | ||
| kubectl -n tenant-test wait dv vm-disk-$name --timeout=150s --for=condition=ready | ||
| kubectl -n tenant-test wait pvc vm-disk-$name --timeout=100s --for=jsonpath='{.status.phase}'=Bound | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup to VM Disk test for proper test isolation.
The VM Disk test doesn't clean up the created resource, which could cause issues with test isolation and resource cleanup.
kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready
kubectl -n tenant-test wait dv vm-disk-$name --timeout=150s --for=condition=ready
kubectl -n tenant-test wait pvc vm-disk-$name --timeout=100s --for=jsonpath='{.status.phase}'=Bound
+ kubectl -n tenant-test delete vmdisks.apps.cozystack.io $name
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @test "Create a VM Disk" { | |
| name='test' | |
| kubectl -n tenant-test get vmdisks.apps.cozystack.io $name || | |
| kubectl create -f - <<EOF | |
| apiVersion: apps.cozystack.io/v1alpha1 | |
| kind: VMDisk | |
| metadata: | |
| name: $name | |
| namespace: tenant-test | |
| spec: | |
| source: | |
| http: | |
| url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img | |
| optical: false | |
| storage: 5Gi | |
| storageClass: replicated | |
| EOF | |
| sleep 5 | |
| kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready | |
| kubectl -n tenant-test wait dv vm-disk-$name --timeout=150s --for=condition=ready | |
| kubectl -n tenant-test wait pvc vm-disk-$name --timeout=100s --for=jsonpath='{.status.phase}'=Bound | |
| } | |
| @test "Create a VM Disk" { | |
| name='test' | |
| kubectl -n tenant-test get vmdisks.apps.cozystack.io $name || | |
| kubectl create -f - <<EOF | |
| apiVersion: apps.cozystack.io/v1alpha1 | |
| kind: VMDisk | |
| metadata: | |
| name: $name | |
| namespace: tenant-test | |
| spec: | |
| source: | |
| http: | |
| url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img | |
| optical: false | |
| storage: 5Gi | |
| storageClass: replicated | |
| EOF | |
| sleep 5 | |
| kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready | |
| kubectl -n tenant-test wait dv vm-disk-$name --timeout=150s --for=condition=ready | |
| kubectl -n tenant-test wait pvc vm-disk-$name --timeout=100s --for=jsonpath='{.status.phase}'=Bound | |
| kubectl -n tenant-test delete vmdisks.apps.cozystack.io $name | |
| } |
🤖 Prompt for AI Agents
In hack/e2e-apps/vminstance.bats around lines 3 to 24, the VM Disk test creates
resources but does not clean them up afterward, risking test isolation issues.
Add commands at the end of the test to delete the created VMDisk resource and
any associated resources like PVCs to ensure proper cleanup after the test runs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/pull-requests.yaml (1)
90-99: Fix YAML trailing spaces and streamlinesystemd-runinvocation.
Remove the trailing spaces on the blank line 99, and consider collapsing the multi-linesudo systemd-runcall into a single line to avoid potential YAML parsing issues.- sudo systemd-run \ - --on-calendar="$(date -d 'now + 24 hours' '+%Y-%m-%d %H:%M:%S')" \ - --unit=rm-workspace-$SANDBOX_NAME \ - rm -rf /tmp/$SANDBOX_NAME + sudo systemd-run --on-calendar="$(date -d 'now + 24 hours' '+%Y-%m-%d %H:%M:%S')" --unit=rm-workspace-$SANDBOX_NAME rm -rf /tmp/$SANDBOX_NAME
🧹 Nitpick comments (5)
.github/workflows/pull-requests.yaml (5)
125-127: Avoid repetition by passingSANDBOX_NAMEvia job outputs.
Currently, each job recalculates and echoesSANDBOX_NAME. Instead, set it as an output inprepare_envusing::set-output, then reference it in downstream jobs'envblocks.
139-141: Consolidate sandbox ID logic.
This repeatedechoofSANDBOX_NAMEappears in multiple jobs. Consider creating a reusable action or leveraging job outputs to DRY this.
151-151: Format matrix list with proper spacing.
Add spaces after commas in theapparray to adhere to YAML style guidelines.- app: [clickhouse,kubernetes,mysql,postgres,virtualmachine,vminstance] + app: [clickhouse, kubernetes, mysql, postgres, virtualmachine, vminstance]
165-167: Rename step for better clarity.
Consider renaming"E2E Apps"to reflect the specific app being tested, e.g."Run E2E tests for ${{ matrix.app }}".
188-189: Optimize cleanup: skip code checkout.
This job doesn’t use repository code for deletion; you can remove the “Checkout code” step to reduce overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-requests.yaml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pull-requests.yaml
[error] 99-99: trailing spaces
(trailing-spaces)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: 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)
107-109: Verifyprepare-envmake target location.
Thismake prepare-envruns in the repo root; if the target lives inpackages/core/testing, you'll need-C packages/core/testing.#!/bin/bash # Check if prepare-env target exists in root Makefile rg -n '^prepare-env:' Makefile
4565748 to
895546d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/pull-requests.yaml (3)
99-99: Remove trailing whitespaceLine 99 contains trailing spaces which trigger YAML lint warnings. Please strip any trailing whitespace on this blank line.
129-130: Standardize job namingThe job key
setup_tenantand display nameSetup tenant-testare slightly inconsistent. Consider renaming tosetup_tenant/Setup Tenantortenant-setup/Setup Tenant Testsfor clarity.
148-159: DRY up repeated job headers and fix YAML list spacingThe
Set sandbox IDand release-label filter are duplicated across multiple jobs—consider a reusable workflow or step-level anchor to reduce repetition. Also, the comma-separated app list lacks spaces after commas (YAML lint warnings).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-requests.yaml(4 hunks)packages/core/testing/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/testing/Makefile
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pull-requests.yaml
[error] 99-99: trailing spaces
(trailing-spaces)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
[warning] 151-151: too few spaces after comma
(commas)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (5)
.github/workflows/pull-requests.yaml (5)
90-98: Verify self-hosted runner compatibility with systemd-run schedulingRelying on
sudo systemd-runto schedule a cleanup 24 h later assumes the runner hosts systemd. Confirm all self-hosted runners support systemd and handle failures gracefully.
107-109: Leverage sandbox workspace for environment setupGood use of the centralized
/tmp/$SANDBOX_NAMEworkspace to runprepare-env. This avoids redundant checkouts and local path assumptions.
125-127: Use consolidated sandbox for Cozystack installationSwitching the install step into
/tmp/$SANDBOX_NAMEremoves the need for a fresh checkout and streamlines CI. Well done.
139-146: Approve tenant setup stepRunning
make test-apps-tenantinside the sandbox is the right approach to isolate tenant E2E tests.
165-167: Approve per-app test invocationSwitching to
make test-apps-${{ matrix.app }}within the sandbox correctly parallelizes app-specific E2E tests.
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
895546d to
b3380d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/pull-requests.yaml (1)
90-117: Consolidated workspace preparation and timer scheduling
Theprepare_envjob enhancements correctly centralize the workspace and schedule cleanup, but note:
- YAMLlint flagged trailing whitespace at line 102; please remove it to satisfy formatting checks.
- Verify that the multi-line
systemd-runinvocations (lines 98–101) are interpreted correctly by the runner shell.
🧹 Nitpick comments (1)
.github/workflows/pull-requests.yaml (1)
155-175: Matrix-basedtest_appsjob
Splitting per-app tests into a matrix is effective for parallel E2E coverage.
Minor YAML style: on line 158, add spaces after each comma in thematrix.applist to satisfy YAMLlint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-requests.yaml(4 hunks)packages/core/testing/Makefile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/testing/Makefile
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pull-requests.yaml
[error] 102-102: trailing spaces
(trailing-spaces)
[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 (3)
.github/workflows/pull-requests.yaml (3)
132-135: Streamlined Cozystack installation in workspace
Removing the checkout step and runningmake install-cozystackdirectly in/tmp/$SANDBOX_NAMEis appropriate and reduces redundancy.
136-153: Newsetup_tenantjob definition
The tenant setup job cleanly depends oninstall_cozystackand reuses the sandbox ID. Steps to setSANDBOX_NAMEand runtest-apps-tenantalign with the matrix approach.
195-208: Cleanup job teardown steps
Renaming to “Tear down sandbox”, removing the workspace, and stopping/resetting timers is correct. The sequence ensures no orphaned resources.
Summary by CodeRabbit
New Features
Chores