[core] Extract Talos package from installer #1724
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReorganized Talos build and asset generation into a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @kvaps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the core codebase by extracting all Talos Linux-related functionality from the existing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the Talos-related functionality out of the installer package and into a new, dedicated talos package. The separation of concerns is clear and the changes are consistent across the repository, including updates to the root Makefile and Dockerfiles. The code move is well-executed. I have one suggestion regarding the implementation in the new packages/core/talos/Makefile to make it more idiomatic and robust by leveraging make's dependency management features.
| image-talos: | ||
| test -f ../../../_out/assets/installer-amd64.tar || make talos-installer | ||
| skopeo copy docker-archive:../../../_out/assets/installer-amd64.tar docker://$(REGISTRY)/talos:$(call settag,$(TALOS_VERSION)) | ||
|
|
||
| image-matchbox: | ||
| test -f ../../../_out/assets/kernel-amd64 || make talos-kernel | ||
| test -f ../../../_out/assets/initramfs-metal-amd64.xz || make talos-initramfs | ||
| docker buildx build -f images/matchbox/Dockerfile ../../.. \ | ||
| --tag $(REGISTRY)/matchbox:$(call settag,$(TAG)) \ | ||
| --tag $(REGISTRY)/matchbox:$(call settag,$(TALOS_VERSION)-$(TAG)) \ | ||
| --cache-from type=registry,ref=$(REGISTRY)/matchbox:latest \ | ||
| --cache-to type=inline \ | ||
| --metadata-file images/matchbox.json \ | ||
| $(BUILDX_ARGS) | ||
| echo "$(REGISTRY)/matchbox:$(call settag,$(TAG))@$$(yq e '."containerimage.digest"' images/matchbox.json -o json -r)" \ | ||
| > ../../extra/bootbox/images/matchbox.tag | ||
| rm -f images/matchbox.json | ||
|
|
||
| assets: talos-iso talos-nocloud talos-metal talos-kernel talos-initramfs | ||
|
|
||
| talos-initramfs talos-kernel talos-installer talos-iso talos-nocloud talos-metal: | ||
| mkdir -p ../../../_out/assets | ||
| cat images/talos/profiles/$(subst talos-,,$@).yaml | \ | ||
| docker run --rm -i -v /dev:/dev --privileged "ghcr.io/siderolabs/imager:$(TALOS_VERSION)" --tar-to-stdout - | \ | ||
| tar -C ../../../_out/assets -xzf- |
There was a problem hiding this comment.
This section of the Makefile uses a test -f ... || make ... pattern combined with phony targets (like talos-installer), which is generally considered a Makefile anti-pattern. It forces make to be called recursively and bypasses its powerful dependency-tracking mechanism. This can lead to unnecessary rebuilds and makes the build process less efficient and harder to debug.
A more idiomatic make approach would be to define rules where the targets are the actual files being generated (e.g., ../../../_out/assets/installer-amd64.tar). The image-talos and image-matchbox targets would then declare dependencies on these files. This allows make to automatically check if the files exist and if their prerequisites have changed, rebuilding them only when necessary.
While this is a larger refactoring, it would significantly improve the robustness and maintainability of this Makefile.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/installer/Makefile (1)
18-28: Declare target as PHONY.The
imagetarget should be declared.PHONY: imagesince it represents an action, not a file. This improves Make behavior and aligns with the static analysis warning.Add a
.PHONYdeclaration at the top of the Makefile:+.PHONY: image image-cozystack pre-checks show apply diff + NAME=installerAlternatively, if a
.PHONYline already exists earlier in the file, addimageto it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(2 hunks)packages/core/installer/Makefile(1 hunks)packages/core/talos/Chart.yaml(1 hunks)packages/core/talos/Makefile(1 hunks)packages/core/talos/images/matchbox/Dockerfile(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/Chart.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Use Helm Charts with the umbrella pattern and vendor upstream charts in
charts/directory
Files:
packages/core/talos/Chart.yaml
🧠 Learnings (3)
📚 Learning: 2025-11-27T11:21:45.808Z
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/Chart.yaml : Use Helm Charts with the umbrella pattern and vendor upstream charts in `charts/` directory
Applied to files:
packages/core/talos/Chart.yaml
📚 Learning: 2025-07-23T09:15:09.658Z
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.658Z
Learning: The `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the team generally avoids modifying vendored code to maintain clean separation from upstream dependencies.
Applied to files:
packages/core/talos/Chart.yaml
📚 Learning: 2025-11-27T11:21:45.808Z
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/charts/** : Do not directly modify upstream charts; use vendoring approach instead
Applied to files:
packages/core/talos/Chart.yaml
🪛 checkmake (0.2.2)
packages/core/talos/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 11-11: Target "image" should be declared PHONY.
(phonydeclared)
[warning] 31-31: Target "assets" should be declared PHONY.
(phonydeclared)
packages/core/installer/Makefile
[warning] 18-18: Target "image" should be declared PHONY.
(phonydeclared)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (6)
packages/core/talos/images/matchbox/Dockerfile (1)
5-6: Path updates correctly align with package refactor.The COPY paths are properly updated to reference the new talos package location while preserving correct destination paths. This is consistent with the overall package extraction.
Makefile (1)
29-29: Build orchestration correctly delegates to talos package.The changes properly route talos image and asset generation through the new dedicated talos package workflow, with appropriate build ordering (talos before installer). The delegation to
make -C packages/core/talosis consistent with the refactor objectives.Also applies to: 45-45
packages/core/talos/Chart.yaml (1)
1-4: Clarify Chart structure against umbrella pattern guideline.Per the coding guidelines for Helm charts, this should follow the umbrella pattern and vendor upstream charts in a
charts/directory. The current minimal Chart.yaml lacks dependencies or acharts/subdirectory. Verify whether this chart is intended to remain a simple package chart or should be structured as an umbrella chart with vendored dependencies.packages/core/talos/Makefile (3)
13-15: Verify image-talos dependency and tar extraction logic.Line 14 tests for the installer tar file but only re-runs
make talos-installerif missing. Confirm that:
- The
talos-installerpattern target correctly generates../../../_out/assets/installer-amd64.tar- This implicit dependency and conditional build is intentional and reliable across clean and incremental builds
Can you verify that the pattern rule at lines 33-37 produces the
installer-amd64.tarfile whentalos-installeris invoked?
17-29: Verify multiple asset dependencies for image-matchbox.Lines 18-19 test for kernel and initramfs assets but only re-run the respective build if missing. Ensure this conditional logic is robust and works correctly in both clean and incremental build scenarios, especially since both assets are required for the image-matchbox target to succeed.
33-37: Verify pattern rule matches intended asset names.The pattern rule
talos-initramfs talos-kernel talos-installer talos-iso talos-nocloud talos-metal:generates multiple asset types, but the logic at line 35 uses$(subst talos-,,$@)to derive the YAML profile filename. Confirm that each of the asset names (initramfs, kernel, installer, iso, nocloud, metal) corresponds to an actual YAML profile inimages/talos/profiles/directory.
| NAME=talos | ||
| NAMESPACE=cozy-system | ||
|
|
||
| TALOS_VERSION=$(shell awk '/^version:/ {print $$2}' images/talos/profiles/installer.yaml) | ||
|
|
||
| include ../../../scripts/common-envs.mk | ||
|
|
||
| update: | ||
| hack/gen-profiles.sh | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add PHONY target declarations.
The Makefile lacks a .PHONY declaration. At minimum, the primary targets (update, image, image-talos, image-matchbox, assets) and the pattern rule targets should be declared as phony to ensure Make executes them even if files with those names exist.
Add a .PHONY declaration near the top of the file:
+.PHONY: update image image-talos image-matchbox assets talos-initramfs talos-kernel talos-installer talos-iso talos-nocloud talos-metal
+
NAME=talos
NAMESPACE=cozy-system📝 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.
| NAME=talos | |
| NAMESPACE=cozy-system | |
| TALOS_VERSION=$(shell awk '/^version:/ {print $$2}' images/talos/profiles/installer.yaml) | |
| include ../../../scripts/common-envs.mk | |
| update: | |
| hack/gen-profiles.sh | |
| .PHONY: update image image-talos image-matchbox assets talos-initramfs talos-kernel talos-installer talos-iso talos-nocloud talos-metal | |
| NAME=talos | |
| NAMESPACE=cozy-system | |
| TALOS_VERSION=$(shell awk '/^version:/ {print $$2}' images/talos/profiles/installer.yaml) | |
| include ../../../scripts/common-envs.mk | |
| update: | |
| hack/gen-profiles.sh |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
In packages/core/talos/Makefile around lines 1 to 10, the Makefile is missing a
.PHONY declaration which can cause Make to treat target names as files; add a
.PHONY line near the top (after variable declarations) listing the primary
targets: update, image, image-talos, image-matchbox, assets, and any pattern
rule targets like image-%. This ensures Make always runs those targets even if
files with those names exist; simply insert a single .PHONY declaration
containing the comma/space-separated target names right after the
NAME/NAMESPACE/TALOS_VERSION variables.
Move Talos-related functionality from packages/core/installer to a separate packages/core/talos package: - Move Talos profiles, matchbox configuration and build scripts - Update installer Makefile to remove Talos-related targets - Update root Makefile to build talos package separately - Update matchbox Dockerfile paths to reference talos package Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
d9eb5fc to
c0e0ef0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/talos/Makefile (1)
1-10: Add .PHONY declaration for all make targets.The Makefile is missing a
.PHONYdeclaration. A prior review flagged this issue. Declare all targets as phony to ensure make executes them even if files with those names exist, and to prevent unintended skipping of build steps.Add this declaration after the includes (after line 6):
include ../../../scripts/common-envs.mk +.PHONY: update image image-talos image-matchbox assets talos-initramfs talos-kernel talos-installer talos-iso talos-nocloud talos-metal + update:
🧹 Nitpick comments (2)
packages/core/installer/Makefile (1)
18-18: Add .PHONY declaration for the image target.Make will treat
imageas a file target without explicit.PHONYdeclaration. This can cause make to skip execution if a file namedimageexists. Declare it as phony to ensure it always runs.Add this declaration near the top of the Makefile:
+.PHONY: image image-cozystack + NAME=installerpackages/core/talos/Makefile (1)
13-19: Consider refactoring the test-file pattern to improve build reliability.Lines 14, 18-19 use
test -f ... || make ...to conditionally invoke recursive make targets. This pattern bypasses make's dependency-tracking mechanism, leading to unnecessary rebuilds and fragile caching.A more idiomatic approach would define rules where targets are the actual generated files (e.g.,
../../../_out/assets/installer-amd64.tar), allowing make to automatically track dependencies and rebuild only when needed. While this is a larger refactoring deferred from a prior review, consider prioritizing it to improve build robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pull-requests.yaml(1 hunks)Makefile(2 hunks)packages/core/installer/Makefile(1 hunks)packages/core/talos/Chart.yaml(1 hunks)packages/core/talos/Makefile(1 hunks)packages/core/talos/images/matchbox/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/talos/images/matchbox/Dockerfile
- Makefile
🧰 Additional context used
📓 Path-based instructions (1)
**/Chart.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Use Helm Charts with the umbrella pattern and vendor upstream charts in
charts/directory
Files:
packages/core/talos/Chart.yaml
🧠 Learnings (2)
📚 Learning: 2025-11-27T11:21:45.808Z
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/Chart.yaml : Use Helm Charts with the umbrella pattern and vendor upstream charts in `charts/` directory
Applied to files:
packages/core/talos/Chart.yaml
📚 Learning: 2025-07-23T09:15:09.658Z
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.658Z
Learning: The `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the team generally avoids modifying vendored code to maintain clean separation from upstream dependencies.
Applied to files:
packages/core/talos/Chart.yaml
🪛 checkmake (0.2.2)
packages/core/installer/Makefile
[warning] 18-18: Target "image" should be declared PHONY.
(phonydeclared)
packages/core/talos/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 11-11: Target "image" should be declared PHONY.
(phonydeclared)
[warning] 31-31: Target "assets" should be declared PHONY.
(phonydeclared)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
packages/core/talos/Chart.yaml (1)
1-4: Chart structure is appropriate for package extraction.The minimal Helm chart v2 manifest is suitable for this new talos package. The placeholder version with auto-generation comment aligns with build-time versioning practices. However, ensure this chart properly integrates with your Helm deployment strategy if this package will be deployed as a standalone Helm release.
.github/workflows/pull-requests.yaml (1)
60-61: LGTM!The workflow step correctly builds the Talos image from the new
packages/core/talospackage directory. This aligns with the PR's objective to extract Talos functionality into a separate package.
Extract Talos-related functionality from `packages/core/installer` into a separate `packages/core/talos` package. This refactoring separates concerns: - The `installer` package now focuses solely on the Cozystack installer - The `talos` package handles all Talos Linux image building and assets generation Changes: - Created new `packages/core/talos` package with Chart.yaml, Makefile, and values.yaml - Moved Talos profiles (initramfs, kernel, iso, installer, metal, nocloud) to talos package - Moved matchbox configuration and Dockerfile to talos package - Moved `hack/gen-profiles.sh` and `hack/gen-versions.sh` scripts to talos package - Updated installer Makefile to remove all Talos-related targets - Updated root Makefile to build talos package separately - Updated matchbox Dockerfile paths to reference talos package ```release-note [core] Extract Talos package from installer into separate packages/core/talos package ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Chores** * Reorganized build system to add a dedicated Talos package with its own image and asset build workflow * Switched asset generation to the new Talos packaging path and simplified build dependency chain * Added Helm chart manifest and streamlined image build/publishing steps for Talos-related artifacts <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Extract Talos-related functionality from `packages/core/installer` into a separate `packages/core/talos` package. This refactoring separates concerns: - The `installer` package now focuses solely on the Cozystack installer - The `talos` package handles all Talos Linux image building and assets generation Changes: - Created new `packages/core/talos` package with Chart.yaml, Makefile, and values.yaml - Moved Talos profiles (initramfs, kernel, iso, installer, metal, nocloud) to talos package - Moved matchbox configuration and Dockerfile to talos package - Moved `hack/gen-profiles.sh` and `hack/gen-versions.sh` scripts to talos package - Updated installer Makefile to remove all Talos-related targets - Updated root Makefile to build talos package separately - Updated matchbox Dockerfile paths to reference talos package ```release-note [core] Extract Talos package from installer into separate packages/core/talos package ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Chores** * Reorganized build system to add a dedicated Talos package with its own image and asset build workflow * Switched asset generation to the new Talos packaging path and simplified build dependency chain * Added Helm chart manifest and streamlined image build/publishing steps for Talos-related artifacts <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## What this PR does Extract Talos-related functionality from `packages/core/installer` into a separate `packages/core/talos` package. This refactoring separates concerns: - The `installer` package now focuses solely on the Cozystack installer - The `talos` package handles all Talos Linux image building and assets generation Changes: - Created new `packages/core/talos` package with Chart.yaml, Makefile, and values.yaml - Moved Talos profiles (initramfs, kernel, iso, installer, metal, nocloud) to talos package - Moved matchbox configuration and Dockerfile to talos package - Moved `hack/gen-profiles.sh` and `hack/gen-versions.sh` scripts to talos package - Updated installer Makefile to remove all Talos-related targets - Updated root Makefile to build talos package separately - Updated matchbox Dockerfile paths to reference talos package ### Release note ```release-note [core] Extract Talos package from installer into separate packages/core/talos package ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Reorganized build system to add a dedicated Talos package with its own image and asset build workflow * Switched asset generation to the new Talos packaging path and simplified build dependency chain * Added Helm chart manifest and streamlined image build/publishing steps for Talos-related artifacts <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Extract Talos-related functionality from
packages/core/installerinto a separatepackages/core/talospackage.This refactoring separates concerns:
installerpackage now focuses solely on the Cozystack installertalospackage handles all Talos Linux image building and assets generationChanges:
packages/core/talospackage with Chart.yaml, Makefile, and values.yamlhack/gen-profiles.shandhack/gen-versions.shscripts to talos packageRelease note
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.