Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request transitions several components from a Kubernetes-based model to a Docker-centric workflow. The changes update multiple Makefiles and Dockerfiles, remove Helm chart and Kubernetes builder configurations, and adjust VM resource settings. Additionally, two new GitHub Actions workflows have been introduced—one for handling pull requests and another for preparing releases based on tag pushes. The modifications redefine target executions and dependency checks to align with the Docker approach. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as "Pull Request"
participant GH as "GitHub Actions"
participant CR as "Container Registry"
participant Build as "Make Build"
participant Test as "Make Test"
PR->>GH: PR event (labeled "ok-to-test")
GH->>GH: Checkout code
GH->>CR: Login with secret credentials
GH->>Build: Execute `make build`
Build-->>GH: Return build results
GH->>Test: Execute `make test`
Test-->>GH: Return test results
sequenceDiagram
participant Tag as "Tag Push"
participant GH as "GitHub Actions"
participant Repo as "Repository"
participant Release as "Release System"
Tag->>GH: Tag push (v*.*.*)
GH->>GH: Check if release exists
alt No release exists
GH->>Repo: Checkout code
GH->>Repo: Run `make build`
GH->>Release: Create draft release & upload assets
GH->>Repo: Delete pushed tag
else Release exists
GH->>GH: Skip release actions
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
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:
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.
Pull Request Overview
This PR adds a new GitHub CI workflow to run end-to-end tests in a Docker container when pull requests are labeled appropriately.
- Added a workflow file to trigger testing on pull request events.
- Configured a job that checks out the code and runs tests using a Docker container.
kvaps
force-pushed
the
e2e-runner
branch
2 times, most recently
from
802ffb7 to
3329102
Compare
.github/workflows/e2e-test.yml
Outdated
|
|
||
| on: | ||
| pull_request: | ||
| types: [labeled, opened, synchronize, reopened] |
There was a problem hiding this comment.
If you use both opened and synchronize, CI will trigger twice when you open a new PR. If you open it with a label, that's one extra trigger.
| types: [labeled, opened, synchronize, reopened] | |
| types: [synchronize, reopened] |
.github/workflows/e2e-test.yml
Outdated
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 1 |
There was a problem hiding this comment.
depth of 1 is the default, no need to specify it
.github/workflows/e2e-test.yml
Outdated
| name: Run Makefile in e2e container | ||
| runs-on: [self-hosted] | ||
|
|
||
| if: contains(github.event.pull_request.labels.*.name, 'ok-to-test') |
There was a problem hiding this comment.
Oh, I see. You want to run CI when PR has a label. Then you'll want to trigger it when it's labeled, too. It's a complicated thing in GitHub CI. :)
kvaps
force-pushed
the
e2e-runner
branch
5 times, most recently
from
448f49e to
6472b8e
Compare
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/testing/Makefile (1)
61-63: Review Docker-Based Sandbox InitializationThe
wait-for-sandboxtarget now launches the sandbox container with Docker using a pinned image digest and mounts the repository workspace via"${PWD}/../../..:/workspace". Please verify that:
- The volume mount correctly maps the desired workspace inside the container.
- The use of the fixed image reference (with digest) is intentional and up-to-date.
- Any commands later relying on the container’s environment (like the helm template in the test target) are properly supported.
Consider extracting the mount path or image reference into variables for easier maintenance if these values are subject to change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/e2e-test.yml(1 hunks)Makefile(1 hunks)packages/core/testing/Makefile(2 hunks)packages/core/testing/images/e2e-sandbox/Dockerfile(1 hunks)packages/core/testing/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/testing/images/e2e-sandbox/Dockerfile
- packages/core/testing/values.yaml
- Makefile
- .github/workflows/e2e-test.yml
🧰 Additional context used
🪛 GitHub Actions: E2E Test in Docker
packages/core/testing/Makefile
[error] 38-38: Command failed with exit code 127. The script 'hack/e2e.sh' was not found.
[error] 43-43: Make command failed with exit code 2.
🪛 GitHub Actions: Pre-Commit Checks
packages/core/testing/Makefile
[error] 17-17: make: *** [Makefile:17: gen-versions-map] Error 1.
kvaps
force-pushed
the
e2e-runner
branch
6 times, most recently
from
3d87f58 to
a0377ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/core/testing/Makefile (1)
1-66: 💡 Verification agent❓ Verification inconclusive
Pipeline Failure – Check for Obsolete Target Reference
The pipeline failure log indicates an error:[error] 17-17: make: *** [Makefile:17: gen-versions-map] Error 1Even though the
gen-versions-maptarget is not visible in the current modifications, please verify whether this target is defined or referenced via an included file (e.g.,../../../scripts/common-envs.mk) or if it has been removed intentionally. Ensuring CI targets are in sync with the Makefile will help avoid unexpected build failures.
Action: Resolve Missing
gen-versions-mapTarget in CI PipelineIt appears the CI pipeline is failing due to a missing
gen-versions-maptarget. A search across the repository—both in the current Makefile and in the included file (../../../scripts/common-envs.mk)—did not reveal any definition for this target. Please verify whether this target’s removal was intentional. If it was removed, update the CI configuration to eliminate any reference to it; otherwise, reinstate or properly define the target in the Makefile (or its included files) so that CI and file targets are in sync.
- Verify the status of the
gen-versions-maptarget in../../../scripts/common-envs.mkor elsewhere in the repo.- If intentional, remove the CI’s reference to this target.
- If not, add the missing definition back.
🧰 Tools
🪛 GitHub Actions: Pre-Commit Checks
[error] 17-17: make: *** [Makefile:17: gen-versions-map] Error 1
🧹 Nitpick comments (1)
packages/core/testing/Makefile (1)
6-7: New Variable ROOT_DIR – Verify Calculation and Environment Portability
The new variableROOT_DIRis computed as:ROOT_DIR = $(dir $(abspath $(firstword $(MAKEFILE_LIST))/../../..))Please verify that this correctly resolves to the repository root in all intended contexts (local development as well as in CI environments), and that the directory structure does not change unexpectedly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/e2e-test.yml(1 hunks)Makefile(1 hunks)packages/core/testing/Makefile(3 hunks)packages/core/testing/images/e2e-sandbox/Dockerfile(1 hunks)packages/core/testing/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Makefile
- packages/core/testing/images/e2e-sandbox/Dockerfile
- .github/workflows/e2e-test.yml
- packages/core/testing/values.yaml
🧰 Additional context used
🪛 GitHub Actions: Pre-Commit Checks
packages/core/testing/Makefile
[error] 17-17: make: *** [Makefile:17: gen-versions-map] Error 1
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run Makefile in e2e container
🔇 Additional comments (2)
packages/core/testing/Makefile (2)
64-65: Sandbox Container Setup – Validate Repository Copy Operation
In thewait-for-sandboxtarget, the container is launched using:docker run -d --rm --name cozy-e2e-sandbox --privileged ghcr.io/cozystack/cozystack/e2e-sandbox:latest@sha256:f41b1e0f76e7820ce87cd3dfa1b680490d0c898105690c79537943f26ab28040 sleep infinity docker cp "${ROOT_DIR}" cozy-e2e-sandbox:/workspacePlease confirm that copying the repository root to
/workspaceinside the container results in the expected directory structure for test execution. This ensures that subsequent commands, such as those executed in thetesttarget, run in the correct context.
39-40:Details
✅ Verification successful
E2E Test Target – Confirm Script Invocation Path
Thetesttarget now executes:docker exec cozy-e2e-sandbox sh -c 'cd /workspace && export COZYSTACK_INSTALLER_YAML=$$(helm template -n cozy-system installer ./packages/core/installer) && hack/e2e.sh'Ensure that the
hack/e2e.shscript exists at the expected location within/workspaceinside the container. If the file path is different (e.g.,./hack/e2e.sh), consider updating the command to use the correct absolute or relative path. Verifying this will prevent runtime errors like exit code 127 in the CI pipeline.
🏁 Script executed:
#!/bin/bash # Verify that the hack/e2e.sh file exists at the repository root, # which would correspond to /workspace/hack/e2e.sh inside the container. find . -type f -path "./hack/e2e.sh"Length of output: 50
E2E Test Path is Correct – Verified
I confirmed that thehack/e2e.shscript exists in the repository root (which maps to/workspace/hack/e2e.shinside the container), so the current command in thetesttarget is correctly configured. No changes are needed.
kvaps
force-pushed
the
e2e-runner
branch
7 times, most recently
from
9d53a53 to
0960e03
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hack/e2e.sh (1)
87-93: Increased VM Resources Allocation: Verify Host Capacity
The QEMU command now allocates 8 virtual CPUs and 16384 MB of memory (up from 4 vCPUs and 8192 MB). This change should help improve performance during tests, but please ensure that the host running these VMs has sufficient resources; otherwise, it may lead to resource exhaustion.packages/core/testing/Makefile (1)
64-67: Sandbox “apply” Target – Hardcoded Image Digest & Repository Copy
Theapplytarget starts the sandbox container with a hardcoded image digest:ghcr.io/cozystack/cozystack/e2e-sandbox:latest@sha256:f41b1e0f76e7820ce87cd3dfa1b680490d0c898105690c79537943f26ab28040Hardcoding the digest can limit flexibility; consider parameterizing this value if future updates are expected. Additionally, the repository is copied into the sandbox via
docker cp "${ROOT_DIR}" "${SANDBOX_NAME}":/workspace—please verify that this step reliably syncs your source files for the e2e tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/e2e-test.yml(1 hunks)Makefile(2 hunks)hack/e2e.sh(1 hunks)packages/core/builder/Chart.yaml(0 hunks)packages/core/builder/Makefile(0 hunks)packages/core/builder/config.toml(0 hunks)packages/core/builder/templates/sandbox.yaml(0 hunks)packages/core/builder/values.yaml(0 hunks)packages/core/installer/Makefile(3 hunks)packages/core/testing/Makefile(3 hunks)packages/core/testing/images/e2e-sandbox/Dockerfile(1 hunks)packages/core/testing/templates/sandbox.yaml(0 hunks)packages/core/testing/values.yaml(1 hunks)
💤 Files with no reviewable changes (6)
- packages/core/builder/config.toml
- packages/core/builder/Makefile
- packages/core/builder/values.yaml
- packages/core/testing/templates/sandbox.yaml
- packages/core/builder/Chart.yaml
- packages/core/builder/templates/sandbox.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/testing/images/e2e-sandbox/Dockerfile
- packages/core/testing/values.yaml
- Makefile
🧰 Additional context used
🪛 GitHub Actions: Pre-Commit Checks
packages/core/testing/Makefile
[error] 17-17: make: *** [Makefile:17: gen-versions-map] Error 1.
packages/core/installer/Makefile
[error] 17-17: make: *** [Makefile:17: gen-versions-map] Error 1.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and run e2e tests
🔇 Additional comments (9)
.github/workflows/e2e-test.yml (3)
3-6: Review Workflow Trigger Events
The workflow is set to trigger onlabeled, opened, synchronize, reopenedevents. Note that previous feedback (see NickVolynkin’s comment) suggested that using bothopenedandsynchronizecan lead to duplicate runs when a PR is opened with a label. Please double-check that these trigger events match your intended CI behavior and consider consolidating triggers if needed.
15-16: Conditional Execution for Label Check
The condition usingif: contains(github.event.pull_request.labels.*.name, 'ok-to-test')ensures that the e2e tests only run when the PR has the
ok-to-testlabel. This is a good safeguard; just confirm that this matches your testing policy.
18-37: Job Steps and Build/Test Commands
The checkout, login, build (make build), and test (make test) steps are straightforward and clear. They properly chain the Docker-based build and test operations. Ensure that the underlying Make targets and Docker image configurations are kept in sync with these CI steps.packages/core/installer/Makefile (4)
25-40: Streamlined Image Build for “image-cozystack”
The updatedimage-cozystacktarget now uses Docker Buildx to build and push the image, and then updatesvalues.yamlwith the new image digest usingyq. This refactoring simplifies the process by removing former dependencies. Ensure that the version ofyqin use is compatible with the syntax employed and that the image digest extraction meets your requirements.
41-44: Image-Talos Target Adjustments
Theimage-talostarget now checks for the tarball and, if missing, calls thetalos-installertarget before copying the image via skopeo. This logic is concise; please verify that the absence of a dependency onrun-builderdoes not affect expected outcomes.
45-60: Refined “image-matchbox” Target
Theimage-matchboxtarget builds the image with Docker Buildx and writes the generated tag to a file. This approach appears correct; however, as these targets are a critical part of the deployment pipeline, please confirm that the new Docker-based commands fully replace the previous Kubernetes job-based method.
20-22: Pipeline Failure – Missing Target Verification
The pre-commit checks report an error referencing:make: *** [Makefile:17: gen-versions-map] Error 1.Although the current diff does not show a
gen-versions-maptarget, please verify that any references to this target have been removed or updated consistently across the build scripts (and thathack/gen-profiles.shdoes not rely on it).packages/core/testing/Makefile (2)
5-7: New Sandbox Variables Definition
The introduction ofSANDBOX_NAMEandROOT_DIRimproves clarity for sandbox container operations and repository path management within Docker. Confirm that these variables correctly reflect the intended directory structure and container naming conventions across environments.
40-42:Details
✅ Verification successful
Updated Test Target: Validate Script Path and Environment
Thetesttarget now runs:docker exec "${SANDBOX_NAME}" sh -c 'cd /workspace && export COZYSTACK_INSTALLER_YAML=$$(helm template -n cozy-system installer ./packages/core/installer) && hack/e2e.sh'Ensure that, once the repository is copied into
/workspace, thehack/e2e.shscript is indeed located at/workspace/hack/e2e.sh. If the file path differs (for example, needing a./hack/e2e.sh), adjust accordingly to prevent a “script not found” error (which has been an issue previously).
Below is a shell script to verify that the
hack/e2e.shfile is located at the expected repository root (so that when the repository is copied into/workspaceit will be available as/workspace/hack/e2e.sh). Please run this to confirm its location:
Critical: Validate the
hack/e2e.shScript Path
Please confirm that when the repository is copied into/workspace, thehack/e2e.shscript is available at/workspace/hack/e2e.sh. If the file actually resides in a different directory (for example, if it should be invoked as./hack/e2e.sh), update the test target accordingly to prevent a “script not found” error.
🏁 Script executed:
#!/bin/bash # Check if hack/e2e.sh exists at the repository root. if [ -f hack/e2e.sh ]; then echo "Found hack/e2e.sh at repository root (it should be at /workspace/hack/e2e.sh after repo copy)." else echo "hack/e2e.sh NOT found at repository root. Listing potential matches:" fd hack/e2e.sh fiLength of output: 345
Validated Test Target: File Path Confirmed
We verified that thehack/e2e.shscript is present at the repository root and will be available at/workspace/hack/e2e.shonce copied into the sandbox. The current test target command correctly references the script location.
kvaps
force-pushed
the
e2e-runner
branch
5 times, most recently
from
edfa18c to
ef509a9
Compare
Signed-off-by: Andrei Kvapil <[email protected]>
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit
New Features
Refactor
Chores