[ci] separate build and testing jobs #1005
Conversation
WalkthroughThe workflow for pull requests was refactored to separate the build and test phases into distinct jobs, introducing artifact upload and download steps to decouple them. Corresponding changes in the Makefile now set the installer YAML environment variable from a pre-generated file rather than generating it inline. The release workflow was updated to run Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request Event
participant Build as build Job
participant Artifact as Artifact Store
participant Test as test Job
PR->>Build: Trigger build job
Build->>Artifact: Upload cozystack-installer.yaml
Build-->>Test: Notify completion
Test->>Artifact: Download cozystack-installer.yaml
Test->>Test: Run tests using installer artifact
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-requests.yaml(1 hunks)packages/core/testing/Makefile(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/pull-requests.yaml
46-46: job "test" needs job "build" which does not exist in this workflow
(job-needs)
🔇 Additional comments (1)
.github/workflows/pull-requests.yaml (1)
63-68: Artifact download step aligns with upload path.The download uses
name: cozystack-installerinto_out/assets/, matching the upload configuration. This is correct.
packages/core/testing/Makefile
Outdated
| ../../../_out/cozystack-installer.yaml: | ||
| mkdir -p ../../../_out | ||
| helm template -n cozy-system installer ../installer > ../../../_out/cozystack-installer.yaml |
There was a problem hiding this comment.
Align installer artifact path with workflow upload directory.
The workflow uploads cozystack-installer.yaml into _out/assets/, but this rule generates it in _out/. This mismatch will cause test-cluster to regenerate the YAML instead of consuming the uploaded artifact.
Proposed fix:
-../../../_out/cozystack-installer.yaml:
- mkdir -p ../../../_out
- helm template -n cozy-system installer ../installer > ../../../_out/cozystack-installer.yaml
+../../../_out/assets/cozystack-installer.yaml:
+ mkdir -p ../../../_out/assets
+ helm template -n cozy-system installer ../installer > ../../../_out/assets/cozystack-installer.yaml📝 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.
| ../../../_out/cozystack-installer.yaml: | |
| mkdir -p ../../../_out | |
| helm template -n cozy-system installer ../installer > ../../../_out/cozystack-installer.yaml | |
| ../../../_out/assets/cozystack-installer.yaml: | |
| mkdir -p ../../../_out/assets | |
| helm template -n cozy-system installer ../installer > ../../../_out/assets/cozystack-installer.yaml |
🤖 Prompt for AI Agents
In packages/core/testing/Makefile around lines 35 to 37, the rule generates the
cozystack-installer.yaml file in the _out/ directory, but the workflow expects
it in _out/assets/. Modify the mkdir command to create the _out/assets directory
and update the output path of the helm template command to
../../../_out/assets/cozystack-installer.yaml to align with the workflow upload
directory.
packages/core/testing/Makefile
Outdated
| test-cluster: ../../../_out/cozystack-installer.yaml ## Run the end-to-end for creating a cluster | ||
| docker exec "${SANDBOX_NAME}" sh -c 'cd /workspace && export COZYSTACK_INSTALLER_YAML=$$(cat ../../../_out/cozystack-installer.yaml) && hack/cozytest.sh hack/e2e-cluster.bats' | ||
|
|
There was a problem hiding this comment.
Update test-cluster to reference the new artifact location.
The prerequisite and cat path must point to _out/assets/cozystack-installer.yaml to match the upload/download layout.
Proposed change:
-test-cluster: ../../../_out/cozystack-installer.yaml ## Run the end-to-end for creating a cluster
- docker exec "${SANDBOX_NAME}" sh -c 'cd /workspace && export COZYSTACK_INSTALLER_YAML=$$(cat ../../../_out/cozystack-installer.yaml) && hack/cozytest.sh hack/e2e-cluster.bats'
+test-cluster: ../../../_out/assets/cozystack-installer.yaml ## Run the end-to-end for creating a cluster
+ docker exec "${SANDBOX_NAME}" sh -c 'cd /workspace && export COZYSTACK_INSTALLER_YAML=$$(cat ../../../_out/assets/cozystack-installer.yaml) && hack/cozytest.sh hack/e2e-cluster.bats'📝 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-cluster: ../../../_out/cozystack-installer.yaml ## Run the end-to-end for creating a cluster | |
| docker exec "${SANDBOX_NAME}" sh -c 'cd /workspace && export COZYSTACK_INSTALLER_YAML=$$(cat ../../../_out/cozystack-installer.yaml) && hack/cozytest.sh hack/e2e-cluster.bats' | |
| test-cluster: ../../../_out/assets/cozystack-installer.yaml ## Run the end-to-end for creating a cluster | |
| docker exec "${SANDBOX_NAME}" sh -c 'cd /workspace && export COZYSTACK_INSTALLER_YAML=$$(cat ../../../_out/assets/cozystack-installer.yaml) && hack/cozytest.sh hack/e2e-cluster.bats' |
🤖 Prompt for AI Agents
In packages/core/testing/Makefile at lines 39 to 41, update the test-cluster
target to reference the new artifact location by changing the path from
../../../_out/cozystack-installer.yaml to
../../../_out/assets/cozystack-installer.yaml in both the prerequisite and the
cat command inside the docker exec line.
| - name: Upload installer artifact | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: cozystack-installer | ||
| path: _out/assets/cozystack-installer.yaml | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that the build step produces the installer at _out/assets.
This upload step assumes make build outputs _out/assets/cozystack-installer.yaml. Please confirm the root Makefile’s build target writes the artifact there, otherwise the upload will fail.
🏁 Script executed:
#!/bin/bash
# Confirm that the build target generates the installer in _out/assets/
rg -n "cozystack-installer.yaml" -n MakefileLength of output: 162
I want to confirm this command is indeed part of the build target in the root Makefile. Let’s grab a few lines of context around it:
🏁 Script executed:
#!/bin/bash
rg -n -B3 -A3 "cozystack-installer.yaml" MakefileLength of output: 269
Let’s confirm that the build target depends on the manifests step (which generates the installer):
🏁 Script executed:
#!/bin/bash
# Show the build target and its prerequisites
rg -n -C3 "^build:" MakefileLength of output: 324
Ensure the installer is generated before uploading
The build target in the root Makefile (line 10) only depends on build-deps and builds container images; it never invokes the manifests rule that renders _out/assets/cozystack-installer.yaml (defined on line 41). As written, the upload step will fail because the installer file isn’t produced.
Please address this by either:
- Adding
manifestsas a prerequisite of thebuildtarget (e.g.build: build-deps manifests), or - Inserting an explicit
make manifests(ormake -C packages/core/installer/ assets) step in your GitHub workflow before the upload.
🤖 Prompt for AI Agents
In .github/workflows/pull-requests.yaml around lines 40 to 45, the upload step
assumes the installer artifact is generated by the build step, but the root
Makefile's build target does not run the manifests rule that creates this
artifact. Fix this by either adding manifests as a prerequisite to the build
target in the Makefile or by adding an explicit step in the GitHub workflow to
run make manifests (or make -C packages/core/installer/ assets) before the
upload step to ensure the installer file exists.
8917e87 to
c16a2fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/pull-requests-release.yaml (1)
38-39: Separate manifest generation and test execution steps.Combining
make manifests && make testinto a single step can make it harder to pinpoint failures. Splitting into two named steps improves CI logs and debugging.Consider this adjustment:
--- a/.github/workflows/pull-requests-release.yaml +++ b/.github/workflows/pull-requests-release.yaml @@ -37,4 +37,9 @@ - name: Login to GitHub Container Registry uses: docker/login-action@v3 @@ - - name: Run tests - run: make manifests && make test + - name: Generate Manifests + run: make manifests + + - name: Run Tests + run: make test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pull-requests-release.yaml(1 hunks).github/workflows/pull-requests.yaml(2 hunks)packages/core/testing/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pull-requests.yaml
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil kvapss@gmail.com
Summary by CodeRabbit