[platform] Migrate from HelmRelease bundles to Package-based deployment #1816
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. 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 introduces a fundamental shift in how the platform's components are deployed and managed. It transitions from a HelmRelease-driven system to a more modular and operator-managed Package resource model. This change aims to streamline the definition, configuration, and lifecycle management of various platform services, offering greater flexibility and consistency across different deployment variants. The update includes comprehensive restructuring of configuration, new resource definitions for various applications, and a migration path for existing setups. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 introduces a significant and well-structured refactoring of the deployment system, migrating from a HelmRelease-based approach to a more modular Package-based system managed by cozystack-operator. The changes are extensive, including a complete overhaul of values.yaml for better configuration management, the introduction of bundle variants, and the creation of a migration script to ease the transition for existing users. This is a great improvement for the project's maintainability. I've identified a critical issue that would break deployments, along with several medium severity issues that should be addressed to improve code quality and robustness. My detailed feedback is in the comments below.
| dependsOn: [cilium] | ||
| values: | ||
| cozystack: | ||
| nodesHash: {{ include "cozystack.master-node-ips" . | sha256sum }} |
There was a problem hiding this comment.
The helper cozystack.master-node-ips used here has been removed from packages/core/platform/templates/_helpers.tpl. This will cause the Helm chart rendering to fail. This line needs to be fixed or removed.
| # This script converts cozystack, cozystack-branding, and cozystack-scheduling | ||
| # ConfigMaps into a Package resource with the new values structure. | ||
|
|
||
| set -e |
There was a problem hiding this comment.
For better script robustness, it's recommended to use set -euo pipefail instead of just set -e.
e: exits the script if a command fails.u: exits the script if an unset variable is used.o pipefail: causes a pipeline to return the exit status of the last command in the pipe that returned a non-zero status.
This will make the script safer and prevent unexpected behavior.
| set -e | |
| set -euo pipefail |
| ROOT_HOST=$(echo "$COZYSTACK_CM" | jq -r '.data["root-host"] // "example.org"') | ||
| API_SERVER_ENDPOINT=$(echo "$COZYSTACK_CM" | jq -r '.data["api-server-endpoint"] // ""') | ||
| OIDC_ENABLED=$(echo "$COZYSTACK_CM" | jq -r '.data["oidc-enabled"] // "false"') | ||
| TELEMETRY_ENABLED=$(echo "$COZYSTACK_CM" | jq -r '.data["telemetry-enabled"] // "true"') |
There was a problem hiding this comment.
The variable TELEMETRY_ENABLED is extracted from the ConfigMap but is not used anywhere in the script. This unused variable should be removed to improve code clarity.
| - matchLabels: | ||
| apps.cozystack.io/user-service: "true" |
There was a problem hiding this comment.
The matchLabels selector apps.cozystack.io/user-service: "true" is very generic. This could lead to unintentionally selecting services from other applications within the same tenant if they happen to use the same label. To prevent such conflicts, it's safer to use a more specific label selector that includes the instance name, for example by adding a label like app.kubernetes.io/instance: "{{ .name }}" to the services created by the vm-instance chart and matching on it here.
| - name: vpc | ||
| path: apps/vpc | ||
| libraries: [cozy-lib] | ||
|
|
||
| - name: virtualprivatecloud | ||
| path: apps/vpc | ||
| libraries: [cozy-lib] |
There was a problem hiding this comment.
There appears to be a duplicate artifact definition. Both vpc and virtualprivatecloud point to the same path apps/vpc. Since there is an ApplicationDefinition named virtualprivatecloud.yaml, the vpc artifact seems redundant and should be removed to avoid confusion.
| path: extra/ingress | ||
| libraries: [cozy-lib] | ||
| - name: ingress-system | ||
| path: system/ingress |
There was a problem hiding this comment.
The path for the ingress-system artifact is system/ingress, which is inconsistent with bundle-full.yaml where it is system/ingress-nginx. Please ensure this path is correct.
5c8a6e2 to
725f703
Compare
0ee5fb0 to
fccd1e8
Compare
7a73887 to
d812b02
Compare
Fix getVersion to parse "0.1.4+abcdef" format (with "+" separator) instead of incorrectly looking for "sha256:" prefix. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
ab73b49 to
282ee7c
Compare
|
record |
282ee7c to
da61548
Compare
Replace the chart field with chartRef for referencing Helm charts via ExternalArtifact resources. This enables the Package controller to manage chart sources centrally. Changes: - Add chartRef field to CozystackResourceDefinition spec - Remove chart field (deprecated) - Remove validation (moved to controller) - Update lineage mapper for new field structure - Regenerate openapi specs Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/retest |
packages/core/platform/crds/cozystack.io_cozystackresourcedefinitions.yaml
Outdated
Show resolved
Hide resolved
packages/core/platform/bundles/system/applicationdefinitions/bootbox.yaml
Outdated
Show resolved
Hide resolved
Remove legacy installer components (cozystack-assets-server, installer.sh script, cozystack container image) in favor of cozystack-operator based deployment. Move migration scripts from scripts/migrations/ to packages/core/platform/images/migrations/ for containerized execution. Add grafana-dashboards image for centralized dashboard management. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Update all CozystackResourceDefinition files to use chartRef with ExternalArtifact instead of OCIRepository sourceRef. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Restructure platform bundles from monolithic files to modular directory structure with separate applicationdefinitions. Add PackageSources for better dependency management and migrate from legacy HelmRepositories to new repository format. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Add dedicated flux-tenants controller with label selector --watch-label-selector=sharding.fluxcd.io/key=tenants to handle tenant workloads separately from platform components. Update all kubernetes app HelmReleases to: - Use chartRef with ExternalArtifact instead of OCIRepository sourceRef - Add sharding.fluxcd.io/key=tenants label - Add cozystack.io/target-cluster-name label Update fluxinstall to parse multiple YAML manifest files and use flux service for storage-adv-addr. Add cozystack-basics package for core tenant/namespace setup. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
da61548 to
22cd8f1
Compare
Replace HelmRelease-based bundle system with Package resources managed by cozystack-operator. Restructure values.yaml with full configuration support.
What this PR does
Release note