[registry] Add application labels and update filtering mechanism #1707
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. WalkthroughAdds application labels to HelmReleases, switches webhook mapping from chart-driven to label-driven, introduces a new CozystackResourceDefinition Helm reconciler that updates HelmRelease chart/source based on CRDs, and provides a migration script to backfill labels into existing HelmReleases. Changes
Sequence DiagramsequenceDiagram
participant M as Manager (controller-runtime)
participant R as CozystackResourceDefinitionHelmReconciler
participant K as Kubernetes API
participant CRD as CozystackResourceDefinition
participant HR as HelmRelease
rect `#f0f7ff`
M->>R: trigger Reconcile(Request)
note right of R: reconcile fetches CRD and updates HelmReleases
end
R->>K: List CozystackResourceDefinitions
K-->>R: CRD list
loop per CRD
R->>CRD: select CRD
R->>K: List HelmReleases with labels (app.kind, app.group, cozystack.io/ui=true)
K-->>R: matching HelmReleases
loop per HelmRelease
R->>R: validate CRD.Release.Chart.Name & SourceRef
R->>HR: compute expected Chart and SourceRef
alt needs change
R->>K: Update HelmRelease (set Chart & SourceRef)
K-->>R: Update result
else no-op
note right of R: skip update
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Code Review
This PR introduces important changes to the application filtering and identification mechanism, moving from relying on Helm charts to using labels. This is a positive step towards greater flexibility and reliability.
During the review, I discovered several critical issues in pkg/registry/apps/application/rest.go that could cause the application to panic due to accessing a nil label map. These vulnerabilities need to be fixed first.
I also left several medium-severity recommendations aimed at improving code readability, performance, and maintainability, including moving string literals to constants, refactoring duplicate code, and optimizing loops.
1436749 to
d14a98c
Compare
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)
pkg/registry/apps/application/rest.go (1)
613-618: Inconsistent validation approach in Delete.The Delete operation uses
shouldIncludeHelmRelease(line 614), which validates based on chart name and sourceRef, rather than the label-based validation used in Get and List operations. This creates inconsistency across REST operations.For consistency with the label-based filtering approach, consider updating Delete to use label validation:
-// Validate that the HelmRelease meets the inclusion criteria -if !r.shouldIncludeHelmRelease(helmRelease) { - klog.Errorf("HelmRelease %s does not match the required chartName and sourceRef criteria", helmReleaseName) +// Validate that the HelmRelease has required labels +if helmRelease.Labels["apps.cozystack.io/application.kind"] != r.kindName || + helmRelease.Labels["apps.cozystack.io/application.group"] != r.gvk.Group { + klog.Errorf("HelmRelease %s does not match the required application labels", helmReleaseName) // Return NotFound error for Application resource return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) }
🧹 Nitpick comments (1)
pkg/registry/apps/application/rest.go (1)
522-555: Consider removing redundant label validations.The Update operation has potentially redundant label checks:
- Lines 522-528: Application metadata labels are set
- Lines 533-540: Labels are validated immediately after being set
- Lines 549-555: Labels are validated again after the Kubernetes update
The first validation (lines 533-540) will always pass since the labels were just set. The second validation (lines 549-555) might be defensive against a validating webhook or admission controller that could modify labels, but this is unusual.
Unless there's a specific reason to expect label modification between setting and updating, consider removing the redundant checks:
helmRelease.Labels["apps.cozystack.io/application.kind"] = r.kindName helmRelease.Labels["apps.cozystack.io/application.group"] = r.gvk.Group helmRelease.Labels["apps.cozystack.io/application.name"] = app.Name -// Note: Annotations from config are not handled as r.releaseConfig.Annotations is undefined - klog.V(6).Infof("Updating HelmRelease %s in namespace %s", helmRelease.Name, helmRelease.Namespace) -// Before updating, ensure the HelmRelease has required labels -// This prevents updating HelmReleases that should not be managed as Applications -if helmRelease.Labels["apps.cozystack.io/application.kind"] != r.kindName || - helmRelease.Labels["apps.cozystack.io/application.group"] != r.gvk.Group { - klog.Errorf("HelmRelease %s does not match the required application labels", helmRelease.Name) - // Return a NotFound error for the Application resource - return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) -} - // Update the HelmRelease in Kubernetes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/controller/cozystackresource_controller.go(4 hunks)internal/lineagecontrollerwebhook/config.go(1 hunks)internal/lineagecontrollerwebhook/controller.go(1 hunks)pkg/registry/apps/application/rest.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
internal/controller/cozystackresource_controller.gointernal/lineagecontrollerwebhook/config.gopkg/registry/apps/application/rest.gointernal/lineagecontrollerwebhook/controller.go
🧠 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 **/charts/** : Do not directly modify upstream charts; use vendoring approach instead
Applied to files:
internal/lineagecontrollerwebhook/config.gointernal/lineagecontrollerwebhook/controller.go
📚 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 **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code
Applied to files:
internal/lineagecontrollerwebhook/config.gointernal/lineagecontrollerwebhook/controller.go
📚 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:
internal/lineagecontrollerwebhook/config.go
🧬 Code graph analysis (3)
internal/controller/cozystackresource_controller.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (3)
CozystackResourceDefinitionList(37-41)CozystackResourceDefinition(27-32)SourceRef(71-80)
internal/lineagecontrollerwebhook/config.go (2)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinition(27-32)internal/lineagecontrollerwebhook/types.go (1)
LineageControllerWebhook(15-23)
internal/lineagecontrollerwebhook/controller.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinition(27-32)
⏰ 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 (12)
internal/lineagecontrollerwebhook/config.go (3)
5-5: LGTM!The
stringsimport is necessary forstrings.TrimSuffixused in the Map function.
16-28: LGTM!The removal of
chartAppMapand simplification ofruntimeConfigaligns with the PR's shift to label-based mapping.
30-55: Add validation for prefix extraction.The prefix computation on line 52 uses
strings.TrimSuffix(hr.Name, appName), which silently returns the original name if it doesn't end withappName. This masks potential naming configuration errors.Consider adding validation to catch cases where the HelmRelease name doesn't follow the expected
<prefix><appName>format:prefix := strings.TrimSuffix(hr.Name, appName) +if prefix == hr.Name { + return "", "", "", fmt.Errorf("cannot map helm release %s/%s: name does not end with application name %s", hr.Namespace, hr.Name, appName) +}internal/lineagecontrollerwebhook/controller.go (1)
26-41: LGTM!The simplification to use only app-based mapping (
appCRDMap) aligns with the removal of chart-based configuration. The hardcoded group"apps.cozystack.io"is consistent with the label keys used throughout the PR.internal/controller/cozystackresource_controller.go (3)
41-62: LGTM!The refactored Reconcile logic now updates HelmReleases for all CRDs and continues processing even if individual updates fail, which improves resilience. The approach ensures that a failure for one CRD doesn't block updates for others.
233-269: LGTM!The label-based filtering approach correctly constructs a selector using
application.kind,application.group, andcozystack.io/ui=true. The hardcoded group"apps.cozystack.io"is consistent across the PR. Error handling appropriately continues processing remaining HelmReleases if one fails.
85-107: Clarify the intent behind the fixed reconcile target in the HelmRelease watch.The watch triggers reconciliation of a fixed target
"cozy-system/cozystack-api"whenever any HelmRelease with thecozystack.io/ui=truelabel changes. This means every labeled HelmRelease update will reconcile the same resource, regardless of which HelmRelease changed.Verify that this design is intentional:
- Is
cozystack-apithe correct reconciliation target, or should it be derived from the watched HelmRelease?- Does triggering the same reconciliation for all labeled HelmRelease changes match your intent, or should the watch be more targeted?
- If this broad approach is by design, consider adding a comment explaining why all CRDs must be resynced on any labeled HelmRelease change.
pkg/registry/apps/application/rest.go (5)
36-36: LGTM!The
selectionimport is necessary for constructing label requirements in the List function.
170-176: LGTM!The Create operation correctly injects application metadata labels (
kind,group,name) onto the HelmRelease. The nil check ensures the labels map is initialized before use.
235-241: LGTM!The Get operation now validates HelmRelease labels directly, replacing the previous
shouldIncludeHelmReleasecheck. This aligns with the PR's label-based filtering approach and correctly returns NotFound when labels don't match.
311-348: LGTM!The List operation correctly constructs label requirements to filter HelmReleases by
application.kindandapplication.group. Always adding these requirements ensures precise filtering, and user-provided labels are properly prefixed before being appended.
367-386: LGTM!The additional label verification loop is good defensive programming. It protects against HelmReleases that might lack proper labels (e.g., created manually or before labels were enforced). The detailed logging helps with troubleshooting.
d14a98c to
d681ba3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/registry/apps/application/rest.go (2)
620-625: Inconsistency: Delete uses chart-based filtering, not label-based.The Delete method still calls
shouldIncludeHelmRelease(line 621) which uses chart/sourceRef matching, while Get, List, and Update now use label-based filtering. This inconsistency could lead to different behavior across CRUD operations.Consider updating Delete to use label-based filtering for consistency:
- // Validate that the HelmRelease meets the inclusion criteria - if !r.shouldIncludeHelmRelease(helmRelease) { - klog.Errorf("HelmRelease %s does not match the required chartName and sourceRef criteria", helmReleaseName) + // Validate that the HelmRelease has required labels + if helmRelease.Labels == nil || helmRelease.Labels[ApplicationKindLabel] != r.kindName || + helmRelease.Labels[ApplicationGroupLabel] != r.gvk.Group { + klog.Errorf("HelmRelease %s does not match the required application labels", helmReleaseName) // Return NotFound error for Application resource return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) }
756-758: Inconsistency: Watch uses chart-based filtering, not label-based.The Watch method still calls
shouldIncludeHelmRelease(line 756) which uses chart/sourceRef matching, while other methods now use label-based filtering. This inconsistency could cause watch events to include/exclude different HelmReleases than the corresponding Get/List operations.Consider updating Watch to use label-based filtering for consistency with other methods.
♻️ Duplicate comments (1)
internal/lineagecontrollerwebhook/config.go (1)
30-40: Good: Helper function addresses previous review feedback.The
getApplicationLabelhelper function has been extracted as suggested in previous reviews, improving code reuse and readability.
🧹 Nitpick comments (5)
internal/controller/cozystackresource_controller.go (4)
52-58: Continue processing on failure may hide systemic issues.The loop continues on individual CRD failures, which is reasonable for resilience. However, the error is logged but never aggregated or returned, making it hard to detect when multiple CRDs are failing. Consider collecting errors and returning a combined error or requeuing with backoff.
+ var errs []error // Update HelmReleases for each CRD for i := range crdList.Items { crd := &crdList.Items[i] if err := r.updateHelmReleasesForCRD(ctx, crd); err != nil { logger.Error(err, "failed to update HelmReleases for CRD", "crd", crd.Name) - // Continue with other CRDs even if one fails + errs = append(errs, err) } } + + if len(errs) > 0 { + // Requeue to retry failed CRDs + return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + }
239-248: Hardcoded application group may limit future extensibility.The
applicationGroupis hardcoded to"apps.cozystack.io"(line 240), but the label selector uses this value to filter HelmReleases. If application groups ever vary by CRD, this would need refactoring. Consider whether this should be derived from the CRD spec.
260-266: Errors in individual HelmRelease updates are silently continued.Similar to the outer loop, errors updating individual HelmReleases are logged but the function returns
nil, making it appear successful even when some updates failed. Consider returning an aggregate error.
295-298: TODO comment for Version and ReconcileStrategy fields.The comment at line 298 indicates these should potentially be configurable. If this is planned work, consider creating a tracking issue.
Do you want me to open an issue to track adding configurable
VersionandReconcileStrategyfields toCozystackResourceDefinitionChart?pkg/registry/apps/application/rest.go (1)
380-393: Redundant safety filter after label selector query.The code applies a safety filter (lines 386-393) after the label selector query. While defensive, if the label selector works correctly, this is redundant. Consider whether this adds value or just overhead for large result sets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/controller/cozystackresource_controller.go(4 hunks)internal/lineagecontrollerwebhook/config.go(1 hunks)internal/lineagecontrollerwebhook/controller.go(1 hunks)pkg/registry/apps/application/rest.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/lineagecontrollerwebhook/controller.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
pkg/registry/apps/application/rest.gointernal/lineagecontrollerwebhook/config.gointernal/controller/cozystackresource_controller.go
🧠 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 **/charts/** : Do not directly modify upstream charts; use vendoring approach instead
Applied to files:
internal/lineagecontrollerwebhook/config.gointernal/controller/cozystackresource_controller.go
📚 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 **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code
Applied to files:
internal/lineagecontrollerwebhook/config.go
🧬 Code graph analysis (1)
internal/controller/cozystackresource_controller.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (3)
CozystackResourceDefinitionList(37-41)CozystackResourceDefinition(27-32)SourceRef(71-80)
🔇 Additional comments (8)
internal/controller/cozystackresource_controller.go (3)
85-107: LGTM on the HelmRelease watch setup.The watch correctly filters HelmReleases by the
cozystack.io/ui=truelabel and triggers reconciliation appropriately. The pattern follows controller-runtime conventions.
283-293: Good: SourceRef validation addresses previous review feedback.The validation for
SourceRef.Kind,SourceRef.Name, andSourceRef.Namespacehas been added as suggested in previous reviews. The implementation correctly logs and skips invalid configurations.
300-326: Good refactor: expectedSourceRef pattern improves readability.The code now constructs an
expectedSourceRefstructure and compares it with the existing one, as suggested in previous reviews. This improves both readability and maintainability.pkg/registry/apps/application/rest.go (5)
71-76: Good: Label constants centralize magic strings.The new exported constants address previous review feedback about duplicated label key strings across the codebase.
177-183: Good: Nil check for labels before assignment.The nil check at line 178 before assigning labels prevents a potential nil map panic, addressing previous review feedback.
242-248: Good: Label-based filtering in Get method.The Get method now validates that the HelmRelease has the required application labels before returning, aligning with the PR's goal of label-based filtering.
318-355: Good: List method enforces label requirements.The List method now builds explicit label requirements for
ApplicationKindLabelandApplicationGroupLabel, ensuring precise filtering of HelmReleases.
987-993: Good: Safe label value accessor.The
getLabelValuehelper function provides safe access to label values with proper nil checking, improving code safety throughout the file.
e410505 to
51a89da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/registry/apps/application/rest.go (1)
71-76: LGTM! Consider sharing these constants across packages.The label constants are well-defined and follow Kubernetes naming conventions. As noted in a previous review, these same label keys are also defined in
internal/lineagecontrollerwebhook/webhook.go. Consider consolidating them into a shared package (e.g.,pkg/apis/apps/v1alpha1) to avoid duplication and reduce the risk of typos.
🧹 Nitpick comments (7)
internal/lineagecontrollerwebhook/config.go (2)
29-42: Consider using label key constants for consistency.The label keys are hardcoded strings here but defined as constants in
webhook.go(ManagerGroupKey,ManagerKindKey,ManagerNameKey). Using the constants would ensure consistency and reduce duplication across the package.- appKind, err := getApplicationLabel(hr, "apps.cozystack.io/application.kind") + appKind, err := getApplicationLabel(hr, ManagerKindKey) if err != nil { return "", "", "", err } - appGroup, err := getApplicationLabel(hr, "apps.cozystack.io/application.group") + appGroup, err := getApplicationLabel(hr, ManagerGroupKey) if err != nil { return "", "", "", err } - appName, err := getApplicationLabel(hr, "apps.cozystack.io/application.name") + appName, err := getApplicationLabel(hr, ManagerNameKey)
10-13: Empty initialization function can be simplified.
initConfignow does nothing. If no future initialization is planned, consider removing theinitOncefield from the struct and this empty function to reduce dead code.scripts/migrations/21 (2)
152-154: Quote the labels variable to prevent word splitting issues.The unquoted
$labelson line 153 relies on word splitting to pass multiple arguments, which can cause issues with special characters in label values. Consider using an array or ensuring values are safe.- kubectl label helmrelease -n "$namespace" "$name" --overwrite $labels + kubectl label helmrelease -n "$namespace" "$name" --overwrite \ + "apps.cozystack.io/application.kind=$app_kind" \ + "apps.cozystack.io/application.group=$app_group" \ + "apps.cozystack.io/application.name=$app_name"
131-132: Consider adding error handling for kubectl/jq pipeline.If
kubectl get helmreleasesfails, the pipeline continues silently. Addingset -o pipefailis already present (line 4), but consider adding explicit error handling or a check that kubectl succeeded.internal/controller/cozystackresource_controller.go (2)
260-268: Consider returning aggregated errors for observability.Individual HelmRelease update errors are logged but the function returns
nileven if some updates fail. For better observability, consider collecting errors and returning an aggregated error or using a multi-error pattern.+ var errs []error // Update each HelmRelease for i := range hrList.Items { hr := &hrList.Items[i] if err := r.updateHelmReleaseChart(ctx, hr, crd); err != nil { logger.Error(err, "failed to update HelmRelease", "name", hr.Name, "namespace", hr.Namespace) + errs = append(errs, err) continue } } - return nil + if len(errs) > 0 { + return fmt.Errorf("failed to update %d HelmReleases", len(errs)) + } + return nil }
244-248: Consider extracting label keys as constants.The label keys
"apps.cozystack.io/application.kind","apps.cozystack.io/application.group", and"cozystack.io/ui"are duplicated across multiple files. Defining these as package-level constants (or importing from a shared location) would improve maintainability.pkg/registry/apps/application/rest.go (1)
664-686: Consider adding application label requirements to Watch selector.The
Listmethod addsApplicationKindLabelandApplicationGroupLabelrequirements to the server-side label selector (lines 318-329), butWatchonly processes user-provided label selectors without adding these requirements. This means Watch receives all HelmReleases matching user selectors and filters them client-side.For consistency and efficiency, consider adding the same application label requirements:
// Process label.selector + // Always add application metadata label requirements + appKindReq, err := labels.NewRequirement(ApplicationKindLabel, selection.Equals, []string{r.kindName}) + if err != nil { + klog.Errorf("Error creating application kind label requirement: %v", err) + return nil, fmt.Errorf("error creating application kind label requirement: %v", err) + } + appGroupReq, err := labels.NewRequirement(ApplicationGroupLabel, selection.Equals, []string{r.gvk.Group}) + if err != nil { + klog.Errorf("Error creating application group label requirement: %v", err) + return nil, fmt.Errorf("error creating application group label requirement: %v", err) + } + labelRequirements := []labels.Requirement{*appKindReq, *appGroupReq} + if options.LabelSelector != nil {This would reduce unnecessary event processing for HelmReleases that will be filtered out anyway.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/controller/cozystackresource_controller.go(4 hunks)internal/lineagecontrollerwebhook/config.go(1 hunks)internal/lineagecontrollerwebhook/controller.go(1 hunks)internal/lineagecontrollerwebhook/matcher.go(0 hunks)internal/lineagecontrollerwebhook/webhook.go(1 hunks)pkg/registry/apps/application/rest.go(12 hunks)scripts/migrations/21(1 hunks)
💤 Files with no reviewable changes (1)
- internal/lineagecontrollerwebhook/matcher.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
internal/lineagecontrollerwebhook/config.gointernal/controller/cozystackresource_controller.gopkg/registry/apps/application/rest.gointernal/lineagecontrollerwebhook/webhook.gointernal/lineagecontrollerwebhook/controller.go
🧠 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 **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code
Applied to files:
internal/lineagecontrollerwebhook/config.go
📚 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:
internal/controller/cozystackresource_controller.go
🧬 Code graph analysis (3)
internal/lineagecontrollerwebhook/config.go (1)
internal/lineagecontrollerwebhook/types.go (1)
LineageControllerWebhook(15-23)
internal/controller/cozystackresource_controller.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (3)
CozystackResourceDefinitionList(37-41)CozystackResourceDefinition(27-32)SourceRef(71-80)
internal/lineagecontrollerwebhook/webhook.go (1)
pkg/apis/core/v1alpha1/tenantresource_types.go (1)
TenantResourceLabelKey(3-3)
🔇 Additional comments (13)
internal/lineagecontrollerwebhook/webhook.go (1)
143-144: Verify intentional removal of tenant resource classification logic.The change unconditionally sets
TenantResourceLabelKeyto"false"for all objects, removing the previous logic that computed this value based on resource selectors. The comment mentions it "can be overridden by other logic if needed," but no such override exists in the current flow.Confirm this is the intended behavior and that tenant resource tracking is no longer required through this webhook, or is handled elsewhere.
internal/lineagecontrollerwebhook/controller.go (1)
7-9: Controller disabled as part of label-based refactor.The controller now returns
nilwithout registering any watches, effectively disabling runtime reconciliation. This aligns with the PR's shift to label-based metadata handling where the controller's previous chart/app mapping logic is no longer needed.internal/lineagecontrollerwebhook/config.go (1)
51-55: Good fix for prefix derivation edge case.The validation ensuring
prefix+appName == hr.Namecorrectly addresses the edge case whereappNameappears multiple times in the HelmRelease name.internal/controller/cozystackresource_controller.go (3)
52-58: Continue-on-error is appropriate for CRD iteration.Processing each CRD independently with
continueon errors ensures one failing CRD doesn't block updates for others. This is a reasonable resilience pattern.
85-107: HelmRelease watch correctly filters by label.The watch only triggers for HelmReleases with
cozystack.io/ui=true, preventing unnecessary reconciliations. The pattern follows controller-runtime conventions.
284-293: Verify if empty Namespace check is necessary given CRD defaults.The code validates that
SourceRef.Namespaceis not empty, but if the CRD schema applies a default of"cozy-public"consistently, this check may be redundant. Confirm whether the default is guaranteed in all code paths, including when CRDs are read from cache or during partial updates, to determine if this validation is still required for defensive programming or can be safely removed.pkg/registry/apps/application/rest.go (7)
177-183: LGTM!The nil check before label assignment is correct, and the application metadata labels are set after merging user labels, ensuring they cannot be overridden.
242-248: LGTM!The nil check before accessing labels addresses the previous review concern. The label validation correctly enforces that only HelmReleases with matching application labels are exposed as Applications.
318-355: LGTM!The label requirement construction is correct. The kind and group requirements are properly enforced, and user-provided label selectors are prefixed before being combined. Error handling is present for requirement creation.
380-398: LGTM! Good defense-in-depth approach.The client-side filtering provides a safety net in case label selectors don't work as expected or HelmReleases were created without proper labels. The nil check on
hr.Labelsprevents panics, and thegetLabelValuehelper provides safe logging.
529-535: LGTM!The label injection follows the same pattern as Create, ensuring consistency. The nil check and label assignment are correct.
603-609: LGTM!The label validation before deletion is consistent with the Get method pattern, preventing deletion of HelmReleases that don't match the expected application labels.
889-895: LGTM!Simple and effective nil-safe helper for retrieving label values, useful for logging purposes.
87133cb to
4b5087b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/lineagecontrollerwebhook/webhook.go (1)
110-145: Ensure tenant resource label is set consistently for all objects
computeLabelssetsTenantResourceLabelKeyto"false"only when owners exist; for theNoAncestorspath it returns a map without this key, despite the comment saying “default”. If downstream logic expects a boolean-ish label ("true"/"false") rather than presence/absence, this inconsistency can be surprising.Consider setting the label in both code paths and folding it into the literal:
func (h *LineageControllerWebhook) computeLabels(ctx context.Context, o *unstructured.Unstructured) (map[string]string, error) { owners := lineage.WalkOwnershipGraph(ctx, h.dynClient, h.mapper, h, o) if len(owners) == 0 { - return map[string]string{ManagedObjectKey: "false"}, NoAncestors + return map[string]string{ + ManagedObjectKey: "false", + corev1alpha1.TenantResourceLabelKey: "false", + }, NoAncestors } ... - labels := map[string]string{ + labels := map[string]string{ ManagedObjectKey: "true", ManagerGroupKey: /* truncated group */, ManagerKindKey: obj.GetKind(), ManagerNameKey: obj.GetName(), + corev1alpha1.TenantResourceLabelKey: "false", } - // Set tenant resource label to false by default (can be overridden by other logic if needed) - labels[corev1alpha1.TenantResourceLabelKey] = "false" return labels, err }This keeps the “default false” semantics explicit for both managed and unmanaged objects.
🧹 Nitpick comments (3)
internal/lineagecontrollerwebhook/controller.go (1)
7-9: No-op controller setup may be confusing; consider documenting or removing
SetupWithManagerAsControllernow just returnsnil, effectively disabling this controller. If this is intentional (webhook-only path), consider either adding a brief comment explaining that it’s intentionally disabled, or removing the method and its wiring entirely to avoid future confusion about an apparently “forgotten” controller hook.internal/controller/cozystackresource_controller.go (1)
41-62: Reconcile now does a full CRD scan per event; watch for scale characteristicsWith the new logic, every reconcile (triggered either by CRD changes or HelmReleases with
cozystack.io/ui=true) lists allCozystackResourceDefinitionobjects and walks them to update matching HelmReleases before running the debounced restart. This is straightforward and correct, but on clusters with many CRDs and HelmReleases it could become a noticeable hot path.If you see reconcile latency or API pressure grow, consider:
- Enqueuing per‑CRD
Reconcilerequests (using the CRD’sNamespacedName) and scopingupdateHelmReleasesForCRDtoreq.Nameinstead of always scanning all CRDs.- Keeping the debounced restart behavior, but decoupling it from the per‑CRD HelmRelease update loop.
For now the implementation is fine; this is mainly something to keep in mind as CRD counts scale.
Also applies to: 69-108
pkg/registry/apps/application/rest.go (1)
71-76: Good centralization of application metadata labels; consider factoring label stamping into a helperDefining
ApplicationKindLabel,ApplicationGroupLabel, andApplicationNameLabelhere and using them in bothCreateandUpdateensures HelmReleases created via the Application API are consistently labeled.Since the exact same label-stamping block appears in both methods, consider extracting it into a small helper (e.g.
func (r *REST) ensureApplicationLabels(hr *helmv2.HelmRelease, app *appsv1alpha1.Application)) to reduce duplication and the risk of the two call sites drifting over time.Also applies to: 173-184, 525-535
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
internal/controller/cozystackresource_controller.go(4 hunks)internal/lineagecontrollerwebhook/config.go(1 hunks)internal/lineagecontrollerwebhook/controller.go(1 hunks)internal/lineagecontrollerwebhook/matcher.go(0 hunks)internal/lineagecontrollerwebhook/webhook.go(1 hunks)packages/apps/tenant/templates/etcd.yaml(1 hunks)packages/apps/tenant/templates/info.yaml(1 hunks)packages/apps/tenant/templates/ingress.yaml(1 hunks)packages/apps/tenant/templates/monitoring.yaml(1 hunks)packages/apps/tenant/templates/seaweedfs.yaml(1 hunks)packages/core/platform/templates/apps.yaml(1 hunks)packages/system/bootbox/templates/bootbox.yaml(1 hunks)pkg/registry/apps/application/rest.go(12 hunks)scripts/migrations/21(1 hunks)
💤 Files with no reviewable changes (1)
- internal/lineagecontrollerwebhook/matcher.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/lineagecontrollerwebhook/config.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
pkg/registry/apps/application/rest.gointernal/lineagecontrollerwebhook/controller.gointernal/controller/cozystackresource_controller.gointernal/lineagecontrollerwebhook/webhook.go
🧠 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 **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code
Applied to files:
internal/lineagecontrollerwebhook/controller.go
📚 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:
internal/controller/cozystackresource_controller.go
🧬 Code graph analysis (1)
internal/lineagecontrollerwebhook/webhook.go (1)
pkg/apis/core/v1alpha1/tenantresource_types.go (1)
TenantResourceLabelKey(3-3)
⏰ 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 (11)
scripts/migrations/21 (1)
1-163: Migration flow and error handling look solidThe script’s structure (
set -euo pipefail, robustdetermine_app_typewith explicit non‑match failure, and validation of both exit status and output before labeling) looks correct and safe for cluster-wide use. Label construction and application viakubectl label --overwritealign with the new application label scheme.internal/controller/cozystackresource_controller.go (1)
233-336: Label-based HelmRelease selection and chart update logic look correct
updateHelmReleasesForCRDbuilds a focused selector onapps.cozystack.io/application.kind,apps.cozystack.io/application.group, andcozystack.io/ui=true, andupdateHelmReleaseChartvalidatesChart.NameandSourceRefbefore creating or updating theSpec.Chartblock. The deep-copy/update pattern and comparison againstexpectedSourceRefavoid unnecessary updates and follow controller-runtime best practices. As per coding guidelines, this is an idiomatic use of controller-runtime and Flux Helm types.pkg/registry/apps/application/rest.go (2)
317-356: List selector composition and defensive label filtering look goodThe
Listimplementation:
- Always injects
ApplicationKindLabel == r.kindNameandApplicationGroupLabel == r.gvk.Groupaslabels.Requirements (viaselection.Equals), and- After listing, re-validates each
HelmRelease’s labels usinggetLabelValueand skips any that don’t match.This two-layer filtering (API-side selector + in-process safety check) is a solid way to guard against mis-labeled or legacy HelmReleases without panicking on nil maps. The new
getLabelValuehelper neatly encapsulates the nil-safe access.Also applies to: 374-392, 889-895
242-248: Label-based gating changes Application API visibility; verify migration coverage for existing HelmReleases
Get,Delete, andWatchoperations now filter HelmReleases based onApplicationKindLabelandApplicationGroupLabel, returning NotFound if these labels are missing or mismatched. This effectively hides any pre-existing HelmReleases without these labels from the Application API.Ensure that
scripts/migrations/21or related chart changes comprehensively cover all HelmReleases that should remain visible under the new label-driven model, and verify there are no unlabeled HelmReleases that would be unexpectedly hidden in production.packages/apps/tenant/templates/info.yaml (1)
7-13: Info HelmRelease labeling aligns with new application metadata schemeThe added
apps.cozystack.io/application.{kind,group,name}labels for the Info HelmRelease are consistent with the new registry expectations and the migration script’sdetermine_app_typemapping (Info/info).packages/core/platform/templates/apps.yaml (1)
37-41: Root-tenant HelmRelease now properly annotated as a Tenant applicationLabeling the
tenant-rootHelmRelease withapplication.kind: Tenantandapplication.group: apps.cozystack.iomakes it discoverable by the new label-based mechanisms in the controller and registry. Theapplication.name: tenant-rootmetadata is consistent with the manifest naming.packages/apps/tenant/templates/monitoring.yaml (1)
5-14: Monitoring HelmRelease labels are consistent with application registry expectationsThe new
apps.cozystack.io/application.{kind,group,name}labels for Monitoring match the application type and the values used elsewhere (migration script, REST). This will allow the controller and registry to correctly associate this HelmRelease with its Application.packages/apps/tenant/templates/etcd.yaml (1)
12-14: Verify label names and values match the registry constants.The three application metadata labels are added correctly to HelmRelease metadata. Please confirm that the label names (
apps.cozystack.io/application.kind,apps.cozystack.io/application.group,apps.cozystack.io/application.name) and values (Etcd,apps.cozystack.io,etcd) match the constants defined inpkg/registry/apps/application/rest.go(ApplicationKindLabel, ApplicationGroupLabel, ApplicationNameLabel).packages/apps/tenant/templates/seaweedfs.yaml (1)
12-14: Confirm "SeaweedFS" capitalization matches registry constants.The three application metadata labels follow the same pattern as other templates. Verify that the
application.kindvalue "SeaweedFS" (with this specific capitalization) matches the constant defined inpkg/registry/apps/application/rest.go.packages/apps/tenant/templates/ingress.yaml (1)
12-14: Labels added consistently with other templates.The three application metadata labels are correctly added and follow the same pattern as etcd.yaml and seaweedfs.yaml. The label values (kind=Ingress, group=apps.cozystack.io, name=ingress) appear consistent with the application metadata framework.
packages/system/bootbox/templates/bootbox.yaml (1)
8-10: Confirm "BootBox" capitalization matches registry constants.The three application metadata labels are correctly added to bootbox.yaml. Verify that the
application.kindvalue "BootBox" (with this specific capitalization) and overall label names and values match the constants defined inpkg/registry/apps/application/rest.go.
lllamnyp
left a comment
There was a problem hiding this comment.
This PR appears to have removed a bit more code than it should have. Especially completely deleting matcher.go should not have been done. Let's discuss at the next opportunity.
|
@lllamnyp Thanks for catching that. I removed matcher.go because those functions weren't used after switching to label-based filtering. Since the resource selector types are still in the API, I can restore the file. Should I bring it back, or is there a specific reason it should stay? |
4b5087b to
6509839
Compare
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)
internal/controller/cozystackresource_controller.go (1)
79-80: Extract hardcoded namespace and name to constants.The namespace
"cozy-system"and name"cozystack-api"are duplicated here and at line 166. Consider defining constants for these values to improve maintainability.Apply this diff to extract constants:
+const ( + cozystackAPINamespace = "cozy-system" + cozystackAPIName = "cozystack-api" +) + func (r *CozystackResourceDefinitionReconciler) SetupWithManager(mgr ctrl.Manager) error { if r.Debounce == 0 { r.Debounce = 5 * time.SecondThen replace the hardcoded values:
return []reconcile.Request{{ NamespacedName: types.NamespacedName{ - Namespace: "cozy-system", - Name: "cozystack-api", + Namespace: cozystackAPINamespace, + Name: cozystackAPIName, }, }}Also applies to: 102-103
♻️ Duplicate comments (2)
internal/controller/cozystackresource_controller.go (2)
52-58: Consider the previously flagged performance concern.The sequential iteration through CRDs was previously noted as a potential bottleneck with many CozystackResourceDefinitions. While the continue-on-error pattern (line 56) is appropriate for fault isolation, consider whether parallel processing with goroutines and
sync.WaitGroupwould improve reconciliation performance.
301-326: Consider simplifying the comparison logic.The condition at line 321 was previously flagged as cumbersome. The expected
SourceRefstructure is now built (lines 301-305), which improves readability. However, the comparison could be further simplified by also extracting the expected chart name.Optional refactor for consistency:
// Build expected SourceRef + expectedChart := crd.Spec.Release.Chart.Name expectedSourceRef := helmv2.CrossNamespaceObjectReference{ Kind: crd.Spec.Release.Chart.SourceRef.Kind, Name: crd.Spec.Release.Chart.SourceRef.Name, Namespace: crd.Spec.Release.Chart.SourceRef.Namespace, } if hrCopy.Spec.Chart == nil { // Need to create Chart spec hrCopy.Spec.Chart = &helmv2.HelmChartTemplate{ Spec: helmv2.HelmChartTemplateSpec{ - Chart: crd.Spec.Release.Chart.Name, + Chart: expectedChart, Version: version, ReconcileStrategy: reconcileStrategy, SourceRef: expectedSourceRef, }, } updated = true } else { // Update existing Chart spec - if hrCopy.Spec.Chart.Spec.Chart != crd.Spec.Release.Chart.Name || + if hrCopy.Spec.Chart.Spec.Chart != expectedChart || hrCopy.Spec.Chart.Spec.SourceRef != expectedSourceRef { - hrCopy.Spec.Chart.Spec.Chart = crd.Spec.Release.Chart.Name + hrCopy.Spec.Chart.Spec.Chart = expectedChart hrCopy.Spec.Chart.Spec.SourceRef = expectedSourceRef updated = true }
🧹 Nitpick comments (2)
internal/controller/cozystackresource_controller.go (2)
240-240: Hardcoded application group may limit flexibility.The
applicationGroupis hardcoded to"apps.cozystack.io". If this needs to vary by CRD or environment, consider reading it from the CRD spec or a configuration source.
296-298: TODO: Consider making version and reconcileStrategy configurable.The hardcoded
version(">= 0.0.0-0") andreconcileStrategy("Revision") are noted in the TODO comment. These defaults are permissive and appropriate for many use cases, but if different CRDs need different strategies, consider adding optional fields toCozystackResourceDefinitionChart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
internal/controller/cozystackresource_controller.go(4 hunks)internal/lineagecontrollerwebhook/config.go(1 hunks)internal/lineagecontrollerwebhook/controller.go(1 hunks)packages/apps/tenant/templates/etcd.yaml(1 hunks)packages/apps/tenant/templates/info.yaml(1 hunks)packages/apps/tenant/templates/ingress.yaml(1 hunks)packages/apps/tenant/templates/monitoring.yaml(1 hunks)packages/apps/tenant/templates/seaweedfs.yaml(1 hunks)packages/core/platform/templates/apps.yaml(1 hunks)packages/system/bootbox/templates/bootbox.yaml(1 hunks)pkg/registry/apps/application/rest.go(12 hunks)scripts/migrations/21(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/apps/tenant/templates/seaweedfs.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps/tenant/templates/etcd.yaml
- scripts/migrations/21
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
pkg/registry/apps/application/rest.gointernal/lineagecontrollerwebhook/config.gointernal/lineagecontrollerwebhook/controller.gointernal/controller/cozystackresource_controller.go
🧠 Learnings (1)
📚 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:
internal/lineagecontrollerwebhook/config.gointernal/lineagecontrollerwebhook/controller.gointernal/controller/cozystackresource_controller.go
🧬 Code graph analysis (2)
internal/lineagecontrollerwebhook/config.go (2)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinition(27-32)internal/lineagecontrollerwebhook/types.go (1)
LineageControllerWebhook(15-23)
internal/controller/cozystackresource_controller.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (3)
CozystackResourceDefinitionList(37-41)CozystackResourceDefinition(27-32)SourceRef(71-80)
🔇 Additional comments (22)
packages/apps/tenant/templates/info.yaml (1)
11-13: LGTM! Application labels added correctly.These labels enable the label-based filtering mechanism introduced in this PR. The values are consistent with the resource type and follow the established pattern.
packages/system/bootbox/templates/bootbox.yaml (1)
8-10: LGTM! Consistent labeling applied.The application metadata labels are correctly structured and will be used by the label-based filtering logic in the registry.
pkg/registry/apps/application/rest.go (10)
36-36: LGTM! Import properly utilized.The
selectionpackage import is correctly used for constructing label requirements in the List method.
71-76: LGTM! Label constants properly defined.Extracting the label keys as public constants is good practice. This centralizes the label key definitions and prevents typos across the codebase.
177-183: LGTM! Proper label injection with nil safety.The application metadata labels are correctly injected after merging system and user labels. The nil check ensures safe map access.
242-248: LGTM! Label validation ensures proper resource filtering.This validation correctly ensures that only HelmReleases with matching application labels are exposed through the API, returning appropriate NotFound errors for non-matching resources.
318-356: LGTM! Robust label selector construction.The approach of always requiring kind and group label selectors ensures precise filtering. The debug logging at line 355 will be helpful for troubleshooting selector issues.
374-393: LGTM! Defensive filtering approach.The safety check at lines 386-392 provides an additional layer of validation, ensuring that only HelmReleases with correct application labels are returned. This is good defensive programming in case label selectors don't filter perfectly.
529-535: LGTM! Consistent label handling in updates.Application labels are correctly maintained during updates, consistent with the Create flow. The nil safety check ensures reliable operation.
603-609: LGTM! Proper validation before deletion.The label validation ensures that only HelmReleases managed as Applications can be deleted through this API, maintaining consistency with the Get method.
740-745: LGTM! Watch events properly filtered.The label-based filtering in the Watch handler is consistent with the List approach, ensuring only relevant HelmReleases generate watch events.
889-895: LGTM! Safe label accessor helper.The
getLabelValuehelper provides safe access to label values with proper nil checks, preventing panics when labels map is nil or keys are missing.packages/apps/tenant/templates/monitoring.yaml (1)
12-14: LGTM! Monitoring labels correctly applied.The application metadata labels follow the established pattern and enable label-based filtering for the Monitoring application.
packages/core/platform/templates/apps.yaml (1)
39-41: LGTM! Root tenant labels properly configured.The application labels for the root tenant are correctly set with kind=Tenant, enabling the label-based reconciliation mechanism.
internal/lineagecontrollerwebhook/controller.go (1)
27-40: LGTM! Clean transition to label-based approach.The removal of chart mapping logic simplifies the reconciliation flow. The appCRDMap initialization remains intact for application-to-CRD lookups, which aligns with the PR's shift to label-based filtering.
packages/apps/tenant/templates/ingress.yaml (1)
12-14: LGTM! Ingress labels correctly applied.The application metadata labels are properly structured and consistent with the labeling pattern used throughout this PR.
internal/lineagecontrollerwebhook/config.go (3)
5-5: LGTM! Clean removal of chart-based mapping.The removal of
chartAppMapand related logic is clean and complete, supporting the PR's transition to label-based application metadata extraction.Also applies to: 17-17, 24-24
30-40: LGTM! Well-designed helper function.The
getApplicationLabelhelper provides safe label extraction with proper nil checks and clear error messages. This addresses the previous review comment about reducing code duplication in label extraction.
42-72: LGTM! Robust label-based mapping with validation.The rewritten
Mapfunction correctly extracts application metadata from labels instead of chart configuration. The validation check at lines 68-70 addresses the edge case where the application name might appear multiple times in the HelmRelease name, ensuring the prefix derivation is always correct.internal/controller/cozystackresource_controller.go (3)
8-8: LGTM!The new imports (
fmtandhelmv2) are necessary for the HelmRelease chart update functionality.Also applies to: 14-14
85-107: LGTM! Clean watch implementation.The HelmRelease watch correctly filters for the
cozystack.io/ui=truelabel and follows controller-runtime patterns. The label check at line 93 prevents unnecessary reconciliations.
278-293: Excellent validation added.The validation for
Chart.Nameand allSourceReffields (Kind, Name, Namespace) addresses the previous review concern. The error is properly logged with context, and the function returns early rather than proceeding with invalid data.✅ Addresses previous comment about validating required SourceRef fields
|
@lllamnyp pr rebased PR Analysis: Migration to Label-based HelmRelease FilteringCommit: Main IdeaMigration from filtering HelmReleases by Detailed Changes by File1.
|
6509839 to
167499b
Compare
167499b to
e293569
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/controller/cozystackresource_controller.go (1)
239-248: Add validation for emptyapplicationKind.If
crd.Spec.Application.Kindis empty, the label selector will match HelmReleases with an emptyapps.cozystack.io/application.kindlabel, potentially causing unintended updates. Add an early return when the kind is empty.// Use application labels to find HelmReleases // Labels: apps.cozystack.io/application.kind and apps.cozystack.io/application.group applicationKind := crd.Spec.Application.Kind + if applicationKind == "" { + logger.V(4).Info("Skipping HelmRelease update: Application.Kind is empty", "crd", crd.Name) + return nil + } applicationGroup := "apps.cozystack.io" // All applications use this group
🧹 Nitpick comments (6)
internal/lineagecontrollerwebhook/config.go (1)
42-73: LGTM! Label-based mapping with proper validation.The refactored
Mapfunction successfully transitions from chart-based to label-based mapping. The prefix validation at lines 68-70 correctly addresses the past review concern about names whereappNameappears multiple times.Consider extracting the hardcoded label keys as constants to ensure consistency across the codebase. The PR summary mentions that constants are defined in
pkg/registry/apps/application/rest.go.const ( ApplicationKindLabel = "apps.cozystack.io/application.kind" ApplicationGroupLabel = "apps.cozystack.io/application.group" ApplicationNameLabel = "apps.cozystack.io/application.name" )internal/controller/cozystackresource_controller.go (5)
51-58: Consider aggregating and returning errors from CRD processing.The loop logs errors but silently continues, returning
nileven if all CRD updates fail. Consider tracking failures and returning an aggregated error or requeuing when errors occur, so the controller-runtime can retry failed reconciliations.+ var errs []error // Update HelmReleases for each CRD for i := range crdList.Items { crd := &crdList.Items[i] if err := r.updateHelmReleasesForCRD(ctx, crd); err != nil { logger.Error(err, "failed to update HelmReleases for CRD", "crd", crd.Name) - // Continue with other CRDs even if one fails + errs = append(errs, err) } } + + // If any CRD updates failed, requeue for retry + if len(errs) > 0 { + return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + }
85-107: Consider using predicates for more efficient event filtering.The current implementation filters HelmReleases in the map function after receiving all events. Using a
builder.WithPredicates()with a label selector predicate would filter at the informer level, reducing unnecessary event processing.import "sigs.k8s.io/controller-runtime/pkg/predicate" // In SetupWithManager: Watches( &helmv2.HelmRelease{}, handler.EnqueueRequestsFromMapFunc(...), builder.WithPredicates(predicate.NewPredicateFuncs(func(obj client.Object) bool { return obj.GetLabels()["cozystack.io/ui"] == "true" })), )
259-268: Error from HelmRelease updates is silently swallowed.The function returns
nileven when individual HelmRelease updates fail. Consider returning an aggregated error or the first error encountered so the caller can decide whether to retry.+ var updateErr error // Update each HelmRelease for i := range hrList.Items { hr := &hrList.Items[i] if err := r.updateHelmReleaseChart(ctx, hr, crd); err != nil { logger.Error(err, "failed to update HelmRelease", "name", hr.Name, "namespace", hr.Namespace) - continue + if updateErr == nil { + updateErr = err + } } } - return nil + return updateErr
328-333: Consider usingPatchinstead ofUpdateto reduce conflicts.The
Updatecall can fail with conflicts if the HelmRelease is modified concurrently. UsingPatchwithclient.MergeFrom(hr.DeepCopy())reduces the likelihood of conflicts by only updating the changed fields.func (r *CozystackResourceDefinitionReconciler) updateHelmReleaseChart(ctx context.Context, hr *helmv2.HelmRelease, crd *cozyv1alpha1.CozystackResourceDefinition) error { logger := log.FromContext(ctx) hrCopy := hr.DeepCopy() + patch := client.MergeFrom(hr.DeepCopy()) updated := false // ... validation and updates ... if updated { logger.V(4).Info("Updating HelmRelease chart", "name", hr.Name, "namespace", hr.Namespace) - if err := r.Update(ctx, hrCopy); err != nil { + if err := r.Patch(ctx, hrCopy, patch); err != nil { return fmt.Errorf("failed to update HelmRelease: %w", err) } }
318-326: Inconsistent handling ofVersionandReconcileStrategybetween creation and update paths.When creating a new Chart spec (lines 309-316),
versionandreconcileStrategyare set. However, when updating an existing Chart (lines 320-325), these fields are not checked or updated. This could lead to inconsistent behavior if defaults need to be enforced.If the intent is to always enforce certain defaults:
} else { // Update existing Chart spec if hrCopy.Spec.Chart.Spec.Chart != crd.Spec.Release.Chart.Name || - hrCopy.Spec.Chart.Spec.SourceRef != expectedSourceRef { + hrCopy.Spec.Chart.Spec.SourceRef != expectedSourceRef || + hrCopy.Spec.Chart.Spec.Version != version || + hrCopy.Spec.Chart.Spec.ReconcileStrategy != reconcileStrategy { hrCopy.Spec.Chart.Spec.Chart = crd.Spec.Release.Chart.Name hrCopy.Spec.Chart.Spec.SourceRef = expectedSourceRef + hrCopy.Spec.Chart.Spec.Version = version + hrCopy.Spec.Chart.Spec.ReconcileStrategy = reconcileStrategy updated = true } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
internal/controller/cozystackresource_controller.go(4 hunks)internal/lineagecontrollerwebhook/config.go(1 hunks)internal/lineagecontrollerwebhook/controller.go(1 hunks)packages/apps/tenant/templates/etcd.yaml(1 hunks)packages/apps/tenant/templates/info.yaml(1 hunks)packages/apps/tenant/templates/ingress.yaml(1 hunks)packages/apps/tenant/templates/monitoring.yaml(1 hunks)packages/apps/tenant/templates/seaweedfs.yaml(1 hunks)packages/core/platform/templates/apps.yaml(1 hunks)packages/system/bootbox/templates/bootbox.yaml(1 hunks)pkg/registry/apps/application/rest.go(12 hunks)scripts/migrations/22(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/registry/apps/application/rest.go
- packages/apps/tenant/templates/ingress.yaml
- packages/apps/tenant/templates/etcd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
internal/lineagecontrollerwebhook/controller.gointernal/controller/cozystackresource_controller.gointernal/lineagecontrollerwebhook/config.go
🧠 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 **/charts/** : Do not directly modify upstream charts; use vendoring approach instead
Applied to files:
internal/lineagecontrollerwebhook/controller.gointernal/controller/cozystackresource_controller.gointernal/lineagecontrollerwebhook/config.go
📚 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 **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code
Applied to files:
internal/lineagecontrollerwebhook/config.go
📚 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:
internal/lineagecontrollerwebhook/config.go
🧬 Code graph analysis (3)
internal/lineagecontrollerwebhook/controller.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinition(27-32)
internal/controller/cozystackresource_controller.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (3)
CozystackResourceDefinitionList(37-41)CozystackResourceDefinition(27-32)SourceRef(71-80)
internal/lineagecontrollerwebhook/config.go (2)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinition(27-32)internal/lineagecontrollerwebhook/types.go (1)
LineageControllerWebhook(15-23)
🔇 Additional comments (13)
packages/apps/tenant/templates/monitoring.yaml (1)
12-14: LGTM! Consistent application labels added.The three application metadata labels are correctly defined and align with the HelmRelease name and the broader label-based filtering strategy introduced in this PR.
packages/apps/tenant/templates/info.yaml (1)
11-13: LGTM! Application labels correctly defined.The labels are consistent with the HelmRelease name and follow the established naming pattern.
packages/apps/tenant/templates/seaweedfs.yaml (1)
12-14: LGTM! Labels align with naming conventions.The application labels are properly defined, with lowercase name matching the HelmRelease and CamelCase kind for the resource type.
internal/lineagecontrollerwebhook/controller.go (1)
26-41: LGTM! Clean simplification of mapping logic.Removing the chart-based mapping (chartAppMap) aligns well with the PR's migration to label-based filtering. The controller now focuses solely on maintaining the appCRDMap, which simplifies the reconciliation logic.
packages/core/platform/templates/apps.yaml (1)
39-41: LGTM! Application labels properly configured.The labels correctly identify this as a Tenant instance named "tenant-root", distinguishing between the application kind and the instance name.
packages/system/bootbox/templates/bootbox.yaml (1)
8-10: LGTM! Consistent labeling applied.The application labels are correctly defined and align with the HelmRelease configuration.
internal/lineagecontrollerwebhook/config.go (1)
30-40: Well-implemented helper function addresses past review feedback.The
getApplicationLabelhelper properly handles nil labels and missing keys with clear error messages. This addresses the previous review comment about extracting duplicate label-reading logic.scripts/migrations/22 (4)
1-6: Clear migration purpose and good error handling setup.The script uses
set -euo pipefailfor robustness and clearly documents its purpose.
9-128: Comprehensive app type detection logic.The
determine_app_typefunction covers a wide range of application types with proper prefix matching. The longest-prefix-first ordering in the case statement is appropriate for handling naming conflicts.
154-154: Good use of --overwrite flag for idempotency.The
--overwriteflag ensures the migration can be safely re-run without errors if labels already exist.
130-156: The migration scope is correct. The namespace filterstartswith("tenant-")includestenant-rootand all tenant-created namespaces, so bootbox and the tenant-root HelmRelease inpackages/core/platform/templates/apps.yamlare both covered by the migration. Additionally, these HelmReleases already have the required application labels configured in their templates, so they are handled correctly by fresh deployments regardless of whether the migration processes them.internal/controller/cozystackresource_controller.go (2)
8-14: LGTM!The new imports are appropriate for the added functionality:
fmtfor error wrapping andhelmv2for HelmRelease API operations.
283-293: LGTM on SourceRef validation.The validation for incomplete SourceRef fields is properly implemented with clear error logging that includes the actual field values for debugging. This addresses the concern from previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/platform/templates/apps.yaml (1)
4-5: Duplicate variable assignment.Lines 4 and 5 both assign
$host := "example.org". This appears to be a pre-existing issue but should be cleaned up.{{- $bundleName := index $cozyConfig.data "bundle-name" }} {{- $bundle := tpl (.Files.Get (printf "bundles/%s.yaml" $bundleName)) . | fromYaml }} -{{- $host := "example.org" }} {{- $host := "example.org" }}
♻️ Duplicate comments (2)
pkg/registry/apps/application/rest.go (2)
70-75: Centralize application label keys to avoid drift across packagesDefining these label keys as exported constants is good, but similar constants already exist in other packages (e.g., the lineage webhook). Consider moving them to a shared location (such as
pkg/apis/apps/v1alpha1or a small labels subpackage) and reusing them everywhere to prevent typos and divergence across controllers, registry, and webhooks.
226-232: Get: label-based gating is nil-safe and consistent with the new contractThe label check on the fetched
HelmReleasecorrectly short-circuits whenLabelsis nil and ensures only releases with matching kind and group are exposed asApplicationobjects. This both avoids the earlier potential nil-map panic and aligns Get with the label-based ownership model used elsewhere.
🧹 Nitpick comments (6)
internal/lineagecontrollerwebhook/config.go (1)
44-57: Consider using label constants for consistency.The label keys are hardcoded as strings here, but
pkg/registry/apps/application/rest.godefines constants (ApplicationKindLabel,ApplicationGroupLabel,ApplicationNameLabel). Consider importing and using those constants to ensure consistency across the codebase.+import ( + "github.com/cozystack/cozystack/pkg/registry/apps/application" +) - appKind, err := getApplicationLabel(hr, "apps.cozystack.io/application.kind") + appKind, err := getApplicationLabel(hr, application.ApplicationKindLabel) if err != nil { return "", "", "", err } - appGroup, err := getApplicationLabel(hr, "apps.cozystack.io/application.group") + appGroup, err := getApplicationLabel(hr, application.ApplicationGroupLabel) if err != nil { return "", "", "", err } - appName, err := getApplicationLabel(hr, "apps.cozystack.io/application.name") + appName, err := getApplicationLabel(hr, application.ApplicationNameLabel)internal/controller/cozystackresource_controller.go (2)
239-255: LGTM!The validation for empty
applicationKindat lines 241-245 addresses the previous review concern. The label selector correctly combines all three required labels for filtering.Consider using label constants from
pkg/registry/apps/application/rest.gofor consistency with other parts of the codebase.
335-340: Consider using Patch instead of Update for concurrent safety.Using
Updatecan fail with conflicts if the HelmRelease is modified concurrently (e.g., by Flux or another controller). Consider usingPatchwithclient.MergeFromfor safer concurrent modifications, similar to the pattern used indebouncedRestart.if updated { logger.V(4).Info("Updating HelmRelease chart", "name", hr.Name, "namespace", hr.Namespace) - if err := r.Update(ctx, hrCopy); err != nil { + patch := client.MergeFrom(hr) + if err := r.Patch(ctx, hrCopy, patch); err != nil { return fmt.Errorf("failed to update HelmRelease: %w", err) } }pkg/registry/apps/application/rest.go (3)
168-175: Factor repeated application label injection into a helperThe logic that ensures
Labelsis non-nil and setsApplicationKindLabel,ApplicationGroupLabel, andApplicationNameLabelin bothCreateandUpdateis correct and enforces the invariants needed for label-based filtering. To keep things DRY and make future label changes safer, consider extracting it into a small helper:@@ - // Add application metadata labels - if helmRelease.Labels == nil { - helmRelease.Labels = make(map[string]string) - } - helmRelease.Labels[ApplicationKindLabel] = r.kindName - helmRelease.Labels[ApplicationGroupLabel] = r.gvk.Group - helmRelease.Labels[ApplicationNameLabel] = app.Name + r.setApplicationMetadataLabels(helmRelease, app) @@ - // Add application metadata labels - if helmRelease.Labels == nil { - helmRelease.Labels = make(map[string]string) - } - helmRelease.Labels[ApplicationKindLabel] = r.kindName - helmRelease.Labels[ApplicationGroupLabel] = r.gvk.Group - helmRelease.Labels[ApplicationNameLabel] = app.Name + r.setApplicationMetadataLabels(helmRelease, app)and add somewhere near the helpers:
+func (r *REST) setApplicationMetadataLabels(hr *helmv2.HelmRelease, app *appsv1alpha1.Application) { + if hr.Labels == nil { + hr.Labels = make(map[string]string) + } + hr.Labels[ApplicationKindLabel] = r.kindName + hr.Labels[ApplicationGroupLabel] = r.gvk.Group + hr.Labels[ApplicationNameLabel] = app.Name +}Also applies to: 483-490
284-321: List: server-side label selector looks correct; avoid re-parsing selector per itemThe construction of
labelRequirementswithApplicationKindLabel/ApplicationGroupLabelplus prefixed user label requirements is sound and matches how HelmRelease labels are written. The extra in-loop label check is a reasonable safety net.One small performance nit: inside the items loop you re-parse
options.LabelSelector.String()for every item. Since you already parsed it earlier to buildprefixedReqs, you can cache the resultinglabels.Selector(for the un-prefixed Application labels) and reuse it in the loop instead of callinglabels.ParseN times.Also applies to: 341-365, 373-399
542-548: Watch: filter correctness is fine, but mirror List’s server-side selector and reuse checksThe watch path enforces
ApplicationKindLabel/ApplicationGroupLabelonly inside the event-processing goroutine, while the server-sideLabelSelectorcurrently only reflects prefixed user labels. This is functionally correct but may deliver more HelmRelease events than necessary for each REST instance.Consider:
- Building the HelmRelease
LabelSelectorinWatchthe same way as inList(always including the kind/group requirements) to push more filtering to the API server.- Reusing a cached
labels.Selectorforoptions.LabelSelectorwhen matchingapp.Labelsinstead of re-parsing the string inside the event loop, mirroring the List refactor suggestion.- Optionally factoring the repeated “has required application labels” check into a small helper, as the same condition now appears in List, Get/Delete, and Watch.
Also applies to: 563-635, 679-683, 698-708
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
internal/controller/cozystackresource_controller.go(4 hunks)internal/lineagecontrollerwebhook/config.go(1 hunks)internal/lineagecontrollerwebhook/controller.go(1 hunks)packages/apps/tenant/templates/etcd.yaml(1 hunks)packages/apps/tenant/templates/info.yaml(1 hunks)packages/apps/tenant/templates/ingress.yaml(1 hunks)packages/apps/tenant/templates/monitoring.yaml(1 hunks)packages/apps/tenant/templates/seaweedfs.yaml(1 hunks)packages/core/platform/templates/apps.yaml(1 hunks)packages/system/bootbox/templates/bootbox.yaml(1 hunks)pkg/registry/apps/application/rest.go(12 hunks)scripts/migrations/22(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/lineagecontrollerwebhook/controller.go
- packages/system/bootbox/templates/bootbox.yaml
- packages/apps/tenant/templates/seaweedfs.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
internal/lineagecontrollerwebhook/config.gopkg/registry/apps/application/rest.gointernal/controller/cozystackresource_controller.go
🧠 Learnings (3)
📓 Common learnings
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
📚 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:
internal/lineagecontrollerwebhook/config.gointernal/controller/cozystackresource_controller.go
📚 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 **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code
Applied to files:
internal/lineagecontrollerwebhook/config.go
🧬 Code graph analysis (3)
internal/lineagecontrollerwebhook/config.go (2)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinition(27-32)internal/lineagecontrollerwebhook/types.go (1)
LineageControllerWebhook(15-23)
pkg/registry/apps/application/rest.go (2)
pkg/apis/apps/v1alpha1/types.go (1)
Application(59-67)pkg/generated/applyconfiguration/apps/v1alpha1/application.go (1)
Application(39-46)
internal/controller/cozystackresource_controller.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (3)
CozystackResourceDefinitionList(37-41)CozystackResourceDefinition(27-32)SourceRef(71-80)
⏰ 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 (16)
packages/apps/tenant/templates/info.yaml (1)
11-13: LGTM!The application labels are correctly added and consistent with the HelmRelease name (
info) and kind (Info). This aligns with the label-based filtering mechanism introduced in this PR.packages/apps/tenant/templates/monitoring.yaml (1)
12-14: LGTM!The application labels are correctly applied and match the HelmRelease name (
monitoring) and kind (Monitoring).packages/apps/tenant/templates/etcd.yaml (1)
12-14: LGTM!The application labels correctly reflect the HelmRelease name (
etcd) and kind (Etcd).packages/core/platform/templates/apps.yaml (1)
39-41: LGTM!The application labels correctly identify the tenant-root HelmRelease with kind
Tenantand nametenant-root, consistent with the label-based filtering approach.packages/apps/tenant/templates/ingress.yaml (1)
12-14: LGTM!The application labels are correctly applied and match the HelmRelease name (
ingress) and kind (Ingress).internal/lineagecontrollerwebhook/config.go (2)
30-40: LGTM!The
getApplicationLabelhelper cleanly handles both nil labels and missing keys with appropriate error messages. This addresses the previous review suggestion for extracting label retrieval logic.
62-72: LGTM!The prefix validation at lines 66-70 correctly addresses the previous review concern about
TrimSuffixbehavior when the application name appears multiple times in the HelmRelease name.scripts/migrations/22 (4)
1-7: LGTM!The script header correctly sets strict error handling with
set -euo pipefailand provides clear documentation of the migration purpose.
9-128: Well-structured application type mapping.The
determine_app_typefunction comprehensively maps HelmRelease names to their corresponding application kinds. The ordering with longer prefixes first (virtual-machine-*beforevm-instance-*, etc.) ensures correct matching. The special cases for exact matches (bootbox,etcd,info,monitoring,seaweedfs) and the hybridingress|ingress-*pattern are handled correctly.
160-162: LGTM!The version stamping correctly creates or updates the ConfigMap with version 23 using the dry-run + apply pattern for idempotency.
137-142: Cannot verify: file not found in repository.The file
scripts/migrations/22does not exist in the current repository. Additionally, I could not locate anydetermine_app_typefunction. Without access to the actual file and complete pipeline context, the validity of this review comment cannot be determined.If this is a review for an in-flight PR or file that will be added, note that the technical claim is correct: POSIX
/bin/shdoes not supportpipefail. However, the concern only applies if the while loop containing these lines is fed by a pipeline (e.g.,command | while read). The snippet alone does not show sufficient context to assess the actual risk.internal/controller/cozystackresource_controller.go (4)
41-62: LGTM!The Reconcile function now correctly iterates over all CozystackResourceDefinitions and updates their associated HelmReleases. The error handling continues processing other CRDs even if one fails, which is appropriate for resilience.
85-107: LGTM!The HelmRelease watch correctly filters for resources with the
cozystack.io/ui=truelabel, ensuring only relevant HelmReleases trigger reconciliation. This follows controller-runtime patterns.
290-300: LGTM!The SourceRef validation at lines 291-300 addresses the previous review concern by checking that all required fields (Kind, Name, Namespace) are non-empty before proceeding.
303-305: Acknowledge the TODO for future improvement.The hardcoded version (
">= 0.0.0-0") and reconcileStrategy ("Revision") are noted with a TODO. This is acceptable for the current implementation, but consider tracking this in an issue if configurability becomes a requirement.pkg/registry/apps/application/rest.go (1)
819-825: Nil-safe label helper is appropriate
getLabelValuecleanly handlesnillabel maps and missing keys, which keeps the logging in List robust and avoids any risk of panics when inspecting labels.
lllamnyp
left a comment
There was a problem hiding this comment.
- We do not trigger on HelmRelease update events (that might cause a storm of mutually triggered reconciles from Flux's helm-controller and our controller).
- We need to factor out the reconciliation of CozyRDs into a separate controller that properly watches for update events on CozyRDs themselves and updates HelmReleases related to the updated CozyRD.
- I still have to review changes to lineage logic and cozystack API.
- Create an issue to move the current controller that restarts the Cozystack API server on CozyRD changes into the Cozystack API server itself.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/controller/cozystackresource_controller.go (2)
196-217: Consider aggregating errors for observability.The function logs errors but always returns
nil, which means the caller cannot distinguish between successful reconciliation and partial failures. While continuing on errors is correct for eventual consistency, consider aggregating errors usingerrors.Join(Go 1.20+) ormultierrorto return a combined error for better observability.🔎 Optional improvement
+import "errors" + func (r *CozystackResourceDefinitionReconciler) reconcileCozyRDAndUpdateHelmReleases(ctx context.Context) error { logger := log.FromContext(ctx) + var errs []error // List all CozystackResourceDefinitions crdList := &cozyv1alpha1.CozystackResourceDefinitionList{} if err := r.List(ctx, crdList); err != nil { logger.Error(err, "failed to list CozystackResourceDefinitions") return err } // Update HelmReleases for each CRD for i := range crdList.Items { crd := &crdList.Items[i] if err := r.updateHelmReleasesForCRD(ctx, crd); err != nil { logger.Error(err, "failed to update HelmReleases for CRD", "crd", crd.Name) - // Continue with other CRDs even if one fails + errs = append(errs, err) } } - return nil + return errors.Join(errs...) }
237-241: Use label constants from the API package.The label keys are hardcoded here, but constants are defined in
pkg/apis/apps/v1alpha1/types.go. Using the constants improves maintainability and prevents typos.🔎 Apply this diff
+ appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1"Then update the label selector:
// Build label selector for HelmReleases // Only reconcile HelmReleases with cozystack.io/ui=true label labelSelector := client.MatchingLabels{ - "apps.cozystack.io/application.kind": applicationKind, - "apps.cozystack.io/application.group": applicationGroup, + appsv1alpha1.ApplicationKindLabel: applicationKind, + appsv1alpha1.ApplicationGroupLabel: applicationGroup, "cozystack.io/ui": "true", }pkg/registry/apps/application/rest.go (1)
591-626: Consider extracting shared label selector logic.The label selector building logic (lines 591-626) duplicates the pattern from
List(lines 284-319). Consider extracting a helper function to reduce duplication and ensure consistency.🔎 Example helper extraction
// buildApplicationLabelSelector creates a label selector combining application metadata requirements // with optional user-provided label requirements func (r *REST) buildApplicationLabelSelector(userLabelSelector labels.Selector) (string, error) { appKindReq, err := labels.NewRequirement(ApplicationKindLabel, selection.Equals, []string{r.kindName}) if err != nil { return "", fmt.Errorf("error creating application kind label requirement: %w", err) } appGroupReq, err := labels.NewRequirement(ApplicationGroupLabel, selection.Equals, []string{r.gvk.Group}) if err != nil { return "", fmt.Errorf("error creating application group label requirement: %w", err) } labelRequirements := []labels.Requirement{*appKindReq, *appGroupReq} // Add prefixed user requirements... return labels.NewSelector().Add(labelRequirements...).String(), nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/cozystackresource_controller.go(3 hunks)pkg/apis/apps/v1alpha1/types.go(1 hunks)pkg/registry/apps/application/rest.go(14 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
pkg/apis/apps/v1alpha1/types.gopkg/registry/apps/application/rest.gointernal/controller/cozystackresource_controller.go
🧠 Learnings (1)
📚 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:
internal/controller/cozystackresource_controller.go
🧬 Code graph analysis (2)
pkg/registry/apps/application/rest.go (3)
pkg/apis/apps/v1alpha1/types.go (4)
ApplicationKindLabel(26-26)ApplicationGroupLabel(27-27)ApplicationNameLabel(28-28)Application(66-74)pkg/registry/core/tenantmodule/rest.go (1)
REST(71-78)pkg/registry/registry.go (1)
REST(26-28)
internal/controller/cozystackresource_controller.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (3)
CozystackResourceDefinitionList(37-41)CozystackResourceDefinition(27-32)SourceRef(71-80)
⏰ 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 (8)
pkg/apis/apps/v1alpha1/types.go (1)
24-29: LGTM! Well-placed API label constants.Defining these constants in the API types package enables consistent usage across the codebase (controller, registry, webhook). The label naming follows Kubernetes conventions with the
apps.cozystack.io/prefix.internal/controller/cozystackresource_controller.go (2)
42-47: Consider the reconciliation efficiency tradeoff.Every reconcile now processes all CozystackResourceDefinitions regardless of which one triggered the event. While this ensures consistency, it may become a performance concern with many CRDs. The current debouncing helps mitigate rapid successive calls, but per past discussion, a future optimization could track which specific CRD triggered the reconcile.
264-329: Good implementation with proper validations.The function properly validates
Chart.NameandSourceReffields before use (addressing prior review feedback). The struct comparison forSourceRefis safe, and the update logic correctly usesDeepCopyto avoid modifying the cached object.The TODO on line 291 for making
VersionandReconcileStrategyconfigurable via CRD is a reasonable future enhancement.pkg/registry/apps/application/rest.go (5)
70-75: LGTM! Proper constant aliasing from API package.This addresses the prior review feedback to define constants in the API package and reference them here.
168-174: LGTM! Correct label injection with nil safety.The nil check ensures the labels map exists before setting application metadata labels. All three labels (kind, group, name) are correctly set.
226-231: LGTM! Proper label-based access control.Using
hasRequiredApplicationLabelsensures that only HelmReleases with matching application metadata are accessible, preventing cross-application type access.
284-321: LGTM! Efficient server-side label filtering.Building the label selector with
labels.NewRequirementand combining with user-provided selectors enables efficient API-level filtering, reducing the amount of data transferred and processed client-side.
766-785: LGTM! Clean helper implementations with proper nil safety.Both helper functions correctly handle the nil Labels case and provide clear, reusable validation logic. The two-tier approach (kind+group vs kind+group+name) provides flexibility for different validation needs.
| // Validate that the HelmRelease has required labels | ||
| if !r.hasRequiredApplicationLabelsWithName(helmRelease, name) { | ||
| klog.Errorf("HelmRelease %s does not match the required application labels", helmReleaseName) | ||
| // Return NotFound error for Application resource | ||
| return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how each CRUD operation validates application labels
echo "=== Searching for hasRequiredApplicationLabels method calls ==="
rg -n "hasRequiredApplicationLabels" pkg/registry/apps/application/rest.go -A 2
echo -e "\n=== Check Get operation around line 227 ==="
sed -n '220,240p' pkg/registry/apps/application/rest.go
echo -e "\n=== Check Update operation ==="
rg -n "func.*Update" pkg/registry/apps/application/rest.go | head -5
echo -e "\n=== Check Delete operation around line 530 ==="
sed -n '525,540p' pkg/registry/apps/application/rest.goRepository: cozystack/cozystack
Length of output: 3055
🏁 Script executed:
#!/bin/bash
# Get the full implementations of both validation methods
echo "=== hasRequiredApplicationLabels implementation ==="
sed -n '766,776p' pkg/registry/apps/application/rest.go
echo -e "\n=== hasRequiredApplicationLabelsWithName implementation ==="
sed -n '776,790p' pkg/registry/apps/application/rest.go
echo -e "\n=== Get Update function to check label validation ==="
sed -n '402,500p' pkg/registry/apps/application/rest.go | head -80
echo -e "\n=== Check what ApplicationNameLabel and other label constants are ==="
rg -n "ApplicationNameLabel|ApplicationKindLabel|ApplicationGroupLabel" pkg/registry/apps/application/rest.go | head -20Repository: cozystack/cozystack
Length of output: 5748
Inconsistent ApplicationNameLabel validation across Get, Update, and Delete operations
Delete validates all three labels (kind, group, and name via hasRequiredApplicationLabelsWithName), while Get (line 227) only validates kind and group via hasRequiredApplicationLabels, missing the name label check. Update inherits this behavior through its Get call, though it explicitly sets the ApplicationNameLabel when updating. Consider whether Get should also validate the name label for consistency, or document the intentional difference.
🤖 Prompt for AI Agents
In pkg/registry/apps/application/rest.go around lines 530 to 535, Delete
validates kind, group and the ApplicationNameLabel via
hasRequiredApplicationLabelsWithName but Get (around line 227) only validates
kind and group; make validation consistent by updating the Get path to also
verify the ApplicationNameLabel (i.e., call
hasRequiredApplicationLabelsWithName(helmRelease, name) instead of
hasRequiredApplicationLabels), update the error handling to return the same
NotFound when the name label is missing, and adjust/add tests and comments to
reflect the tightened validation.
- Add three application metadata labels to HelmRelease: - apps.cozystack.io/application.kind - apps.cozystack.io/application.group - apps.cozystack.io/application.name - Replace shouldIncludeHelmRelease filtering with label-based filtering in Get, List, and Update methods - Always add kind and group label requirements in List for precise filtering - Update CozystackResourceDefinitionController to watch only HelmReleases with cozystack.io/ui=true label - Update LineageControllerWebhook to extract metadata directly from HelmRelease labels instead of using mapping configuration - Add functionality to update HelmRelease chart from CozystackResourceDefinition using label selectors Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
e0e8807 to
7e7716a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/registry/apps/application/rest.go (1)
226-231: Inconsistent ApplicationNameLabel validation across CRUD operations.The Get operation validates only
ApplicationKindLabelandApplicationGroupLabelviahasRequiredApplicationLabels, but Delete (line 531) validates all three labels includingApplicationNameLabelviahasRequiredApplicationLabelsWithName. While Get fetches the resource by exact name (line 210), this inconsistency creates confusion and could lead to unexpected behavior if HelmReleases have incorrect name labels.For consistency and maintainability, consider validating the name label in Get as well.
🔎 Suggested fix to make validation consistent
// Check if HelmRelease has required labels - if !r.hasRequiredApplicationLabels(helmRelease) { + if !r.hasRequiredApplicationLabelsWithName(helmRelease, name) { klog.Errorf("HelmRelease %s does not match the required application labels", helmReleaseName) // Return a NotFound error for the Application resource return nil, apierrors.NewNotFound(r.gvr.GroupResource(), name)Note: This issue was previously identified in past review comments, but the suggestion applies to the current code.
🧹 Nitpick comments (3)
internal/lineagecontrollerwebhook/config.go (1)
44-57: Use label constants instead of hardcoded strings.The label keys are hardcoded in lines 44, 49, and 54. According to the PR objectives, label constants (
ApplicationKindLabel,ApplicationGroupLabel,ApplicationNameLabel) were added topkg/apis/apps/v1alpha1/types.go. Use these constants instead to improve maintainability and ensure consistency across the codebase.🔎 Verify label constants exist and their usage
#!/bin/bash # Description: Verify the label constants exist in the types file and find their usage # Check if the label constants are defined rg -n "ApplicationKindLabel|ApplicationGroupLabel|ApplicationNameLabel" --type go # Check current usage of hardcoded label strings in this file rg -n "apps\.cozystack\.io/application\.(kind|group|name)" internal/lineagecontrollerwebhook/config.gointernal/controller/cozystackresource_controller.go (1)
237-241: Use label constants instead of hardcoded strings.The label selector uses hardcoded label key strings. Per the PR objectives, label constants (
ApplicationKindLabel,ApplicationGroupLabel) were added topkg/apis/apps/v1alpha1/types.go. Import and use these constants for consistency and maintainability.pkg/registry/apps/application/rest.go (1)
766-785: Helpers implemented correctly with proper nil checks.Both helper functions correctly validate application labels with appropriate nil checks. The two-tier approach (
hasRequiredApplicationLabelsfor kind+group only, andhasRequiredApplicationLabelsWithNamefor all three) suggests an intentional design to support different validation strictness levels across operations.However, the inconsistent usage across CRUD operations (Get uses the less strict version, Delete uses the stricter version) may lead to maintenance confusion.
Consider documenting why different operations use different validation levels, or standardize on the stricter
hasRequiredApplicationLabelsWithNamefor all operations to improve consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
internal/controller/cozystackresource_controller.go(3 hunks)internal/lineagecontrollerwebhook/config.go(1 hunks)internal/lineagecontrollerwebhook/controller.go(1 hunks)packages/apps/tenant/templates/etcd.yaml(1 hunks)packages/apps/tenant/templates/info.yaml(1 hunks)packages/apps/tenant/templates/ingress.yaml(1 hunks)packages/apps/tenant/templates/monitoring.yaml(1 hunks)packages/apps/tenant/templates/seaweedfs.yaml(1 hunks)packages/core/platform/templates/apps.yaml(1 hunks)packages/system/bootbox/templates/bootbox.yaml(1 hunks)pkg/apis/apps/v1alpha1/types.go(1 hunks)pkg/registry/apps/application/rest.go(14 hunks)scripts/migrations/22(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/apis/apps/v1alpha1/types.go
- packages/system/bootbox/templates/bootbox.yaml
- scripts/migrations/22
- packages/apps/tenant/templates/info.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
internal/lineagecontrollerwebhook/controller.gopkg/registry/apps/application/rest.gointernal/lineagecontrollerwebhook/config.gointernal/controller/cozystackresource_controller.go
🧠 Learnings (1)
📚 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:
internal/lineagecontrollerwebhook/controller.gointernal/lineagecontrollerwebhook/config.gointernal/controller/cozystackresource_controller.go
🧬 Code graph analysis (3)
internal/lineagecontrollerwebhook/controller.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinition(27-32)
internal/lineagecontrollerwebhook/config.go (2)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinition(27-32)internal/lineagecontrollerwebhook/types.go (1)
LineageControllerWebhook(15-23)
internal/controller/cozystackresource_controller.go (2)
api/v1alpha1/cozystackresourcedefinitions_types.go (3)
CozystackResourceDefinitionList(37-41)CozystackResourceDefinition(27-32)SourceRef(71-80)pkg/apis/apps/v1alpha1/types.go (1)
Application(66-74)
🔇 Additional comments (22)
packages/apps/tenant/templates/seaweedfs.yaml (1)
12-14: LGTM! Label metadata additions are consistent.The application labels correctly identify this HelmRelease: the
application.namematches the HelmRelease name and chart name, and theapplication.kindfollows the PascalCase convention.packages/apps/tenant/templates/ingress.yaml (1)
12-14: LGTM! Label metadata additions are consistent.The application labels correctly identify this HelmRelease with proper naming conventions.
packages/apps/tenant/templates/monitoring.yaml (1)
12-14: LGTM! Label metadata additions are consistent.The application labels correctly identify this HelmRelease with proper naming conventions.
packages/apps/tenant/templates/etcd.yaml (1)
12-14: LGTM! Label metadata additions are consistent.The application labels correctly identify this HelmRelease with proper naming conventions.
internal/lineagecontrollerwebhook/controller.go (1)
26-41: LGTM! Simplification aligns with label-based filtering.The removal of
chartAppMapand chart-based mapping logic correctly supports the PR's shift to label-based filtering. The simplified reconciliation flow now only maintainsappCRDMapindexed by application kind and group, which aligns with the new label-based approach.packages/core/platform/templates/apps.yaml (1)
39-41: LGTM! Label metadata additions are consistent.The application labels correctly identify this tenant-root HelmRelease with proper naming conventions.
internal/lineagecontrollerwebhook/config.go (2)
30-40: LGTM! Helper function correctly handles edge cases.The
getApplicationLabelhelper properly validates nil labels and missing keys with clear error messages. This addresses the code duplication concern from previous reviews.
62-70: LGTM! Prefix validation correctly handles edge cases.The prefix derivation and validation logic properly handles cases where the application name might appear multiple times in the HelmRelease name (e.g., "myapp-myapp"). The validation ensures correctness by verifying that
prefix+appName == hr.Name, addressing the concern raised in previous reviews.internal/controller/cozystackresource_controller.go (6)
42-48: LGTM! Integration of HelmRelease reconciliation is correctly placed.The new reconciliation flow is appropriately invoked at the start of
Reconcilebefore the debounced restart logic, ensuring HelmReleases are synchronized with CRD changes.
196-217: Consider the error handling strategy for CRD reconciliation.The method always returns
nileven if individual CRD updates fail (errors are logged but not propagated). While this fire-and-forget approach prevents blocking the main reconciliation loop, it means failures won't trigger a requeue. Consider whether critical failures should be surfaced to trigger reconciliation retry.As noted in past reviews, this approach reconciles all CRDs on every event, even when only one CRD changed. Consider whether this performance pattern is acceptable for your scale, or if a more targeted reconciliation approach would be beneficial.
227-231: LGTM! Application kind validation correctly prevents invalid selectors.The validation ensures
applicationKindis non-empty before building the label selector, addressing the concern from previous reviews. The error is logged with context, and the function returns early to skip the invalid CRD.
277-286: LGTM! SourceRef validation prevents incomplete configuration.The validation checks all required SourceRef fields (Kind, Name, Namespace) before use, addressing the concern from previous reviews. The error includes detailed context for debugging.
288-292: Acknowledge the TODO for configurable version and reconcileStrategy.The hardcoded defaults (
">= 0.0.0-0"for version and"Revision"for reconcileStrategy) are currently acceptable, and the TODO comment correctly identifies that these should be made configurable via the CRD spec in the future if needed.
294-319: LGTM! Chart update logic is clean and efficient.The expected SourceRef construction and comparison logic correctly addresses the previous review suggestion. The struct comparison works properly for
CrossNamespaceObjectReference, and the single update call (lines 321-326) minimizes API overhead. The logic appropriately handles both Chart creation and update scenarios.pkg/registry/apps/application/rest.go (8)
35-35: LGTM!The
selectionpackage import is correctly added to support the new label requirement construction used in List and Watch operations.
169-174: LGTM!The label initialization and assignment correctly ensures the map is non-nil before setting the three application metadata labels. The placement after label merging prevents these critical labels from being overwritten.
284-321: LGTM!The label selector construction properly enforces server-side filtering on
ApplicationKindLabelandApplicationGroupLabel. User-provided labels are correctly prefixed, and the combined selector ensures efficient filtering at the API server level.
346-390: LGTM!The List processing correctly relies on server-side label selector filtering for kind and group requirements. The client-side filtering loop properly handles Application-level field and label selectors after conversion. The implementation is sound.
471-477: Label injection is correct, but validation is inconsistent with Delete.The Update operation properly sets all three application labels. However, since Update retrieves the existing resource via Get (line 404), it inherits Get's behavior of not validating
ApplicationNameLabel. This creates an inconsistency with Delete, which validates all three labels.Consider whether Update should perform the same stricter validation as Delete for consistency.
Observation: This validation inconsistency is related to the Get operation comment above and was noted in previous reviews.
530-535: LGTM!Delete correctly validates all three application labels (
kind,group, andname) usinghasRequiredApplicationLabelsWithName. This strict validation is appropriate for deletion operations and prevents accidental removal of incorrectly labeled resources.Note: This is the most comprehensive validation among CRUD operations. Consider applying this same level of validation to Get and Update for consistency (as noted in previous comments).
590-626: LGTM!The Watch label selector construction mirrors the List implementation, properly enforcing server-side filtering on
ApplicationKindLabelandApplicationGroupLabel. The consistency between List and Watch operations is excellent.
70-75: All label constants are properly defined in the API package.The three label constants (
ApplicationKindLabel,ApplicationGroupLabel,ApplicationNameLabel) are correctly defined inpkg/apis/apps/v1alpha1/types.goand properly referenced inrest.go. The code change successfully addresses the previous review feedback.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/controller/cozystackresourcedefinition_helmreconciler.go (3)
88-97: Consider aggregating errors for partial failures.The loop logs errors for individual HelmRelease updates but continues and returns
nileven when some updates fail. This means partial failures are silently treated as success from the reconciler's perspective.🔎 Proposed fix to aggregate errors
+ var errs []error // Update each HelmRelease for i := range hrList.Items { hr := &hrList.Items[i] if err := r.updateHelmReleaseChart(ctx, hr, crd); err != nil { logger.Error(err, "failed to update HelmRelease", "name", hr.Name, "namespace", hr.Namespace) - continue + errs = append(errs, err) } } - return nil + return errors.Join(errs...)Add import:
"errors"
112-122: Inconsistent error handling for invalid SourceRef.The function logs an error for incomplete SourceRef but returns
nil, making it indistinguishable from a successful no-op. If this is truly an error condition (as thelogger.Errorsuggests), consider returning an error to surface the misconfiguration. Alternatively, if this is expected and should be silently skipped, uselogger.V(4).Infoinstead oflogger.Errorfor consistency with the Chart.Name check above.🔎 Option: Return error for invalid configuration
if crd.Spec.Release.Chart.SourceRef.Kind == "" || crd.Spec.Release.Chart.SourceRef.Name == "" || crd.Spec.Release.Chart.SourceRef.Namespace == "" { - logger.Error(fmt.Errorf("invalid SourceRef in CRD"), "Skipping HelmRelease chart update: SourceRef fields are incomplete", + return fmt.Errorf("invalid SourceRef in CRD %s: kind=%q, name=%q, namespace=%q", + crd.Name, "crd", crd.Name, - "kind", crd.Spec.Release.Chart.SourceRef.Kind, - "name", crd.Spec.Release.Chart.SourceRef.Name, - "namespace", crd.Spec.Release.Chart.SourceRef.Namespace) - return nil + crd.Spec.Release.Chart.SourceRef.Kind, + crd.Spec.Release.Chart.SourceRef.Name, + crd.Spec.Release.Chart.SourceRef.Namespace) }
157-162: Consider using Patch instead of Update to avoid conflicts.
Update()replaces the entire resource and can fail with conflicts if another controller (e.g., Flux's helm-controller) modifies the HelmRelease concurrently. Using a strategic merge patch or server-side apply is more resilient in multi-controller environments.🔎 Proposed fix using Patch
if updated { logger.V(4).Info("Updating HelmRelease chart", "name", hr.Name, "namespace", hr.Namespace) - if err := r.Update(ctx, hrCopy); err != nil { + patch := client.MergeFrom(hr) + if err := r.Patch(ctx, hrCopy, patch); err != nil { return fmt.Errorf("failed to update HelmRelease: %w", err) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/cozystack-controller/main.gointernal/controller/cozystackresource_controller.gointernal/controller/cozystackresourcedefinition_helmreconciler.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
cmd/cozystack-controller/main.gointernal/controller/cozystackresource_controller.gointernal/controller/cozystackresourcedefinition_helmreconciler.go
🧠 Learnings (1)
📚 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:
internal/controller/cozystackresource_controller.go
🧬 Code graph analysis (1)
cmd/cozystack-controller/main.go (2)
internal/controller/cozystackresourcedefinition_helmreconciler.go (1)
CozystackResourceDefinitionHelmReconciler(24-27)pkg/apiserver/apiserver.go (1)
Scheme(53-53)
⏰ 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)
internal/controller/cozystackresource_controller.go (1)
39-43: LGTM!The comments clearly document the separation of concerns between the debounced restart logic in this reconciler and the HelmRelease reconciliation now handled by
CozystackResourceDefinitionHelmReconciler. This follows good controller-runtime patterns for single-responsibility controllers.internal/controller/cozystackresourcedefinition_helmreconciler.go (4)
17-18: LGTM on RBAC markers.The kubebuilder RBAC annotations correctly declare the required permissions for reading CozystackResourceDefinitions and updating HelmReleases.
29-46: LGTM on Reconcile function.The reconcile flow correctly handles the not-found case and delegates to
updateHelmReleasesForCRD. Returning the error will trigger controller-runtime's default exponential backoff for transient failures.
48-53: Good design choice avoiding HelmRelease watches.By only watching
CozystackResourceDefinitionand not HelmReleases, this reconciler correctly avoids mutual reconciliation storms with Flux's helm-controller (as documented in lines 22-23).
147-155: LGTM on struct comparison and update logic.The direct struct comparison with
!=works correctly forCrossNamespaceObjectReferencesince all its fields are comparable types. The update logic properly handles both creation and update scenarios.cmd/cozystack-controller/main.go (1)
232-238: LGTM!The new
CozystackResourceDefinitionHelmReconcilersetup follows the established pattern used for other controllers in this file. The placement afterCozystackResourceDefinitionReconcileris logical given their related responsibilities.
This change is extracted from
and reworked to work standalone
What this PR does
This PR extracts changes from the #1641. It adds application metadata labels to HelmReleases and updates the filtering mechanism to use labels instead of chart/sourceRef matching.
Changes:
apps.cozystack.io/application.kind,apps.cozystack.io/application.group,apps.cozystack.io/application.name) when creating/updating HelmRelease via Cozystack-APIshouldIncludeHelmReleasefiltering with label-based filtering in Get, List, and Update methodscozystack.io/ui=truelabelRelease note
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.