[kubernetes] Refactor controlPlane settings - #866
Conversation
WalkthroughThe changes consolidate and rename the Kubernetes control plane resource configuration across documentation, schema, Helm templates, and values files. The configuration section previously named Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant values.yaml
participant Helm Chart
participant Kubernetes Cluster
User->>values.yaml: Defines controlPlane resource settings
values.yaml->>Helm Chart: Supplies controlPlane configuration
Helm Chart->>Kubernetes Cluster: Applies resource presets and limits for apiServer, controllerManager, scheduler, konnectivity.server
sequenceDiagram
participant User
participant values.yaml
participant HelmRelease Template
participant Kubernetes Cluster
User->>values.yaml: Defines addon override values
values.yaml->>HelmRelease Template: Supplies override values inline under `values:` key
HelmRelease Template->>Kubernetes Cluster: Deploys addon with inline override values
Suggested labels
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/apps/kubernetes/README.md (2)
62-69: Fix grammatical agreement in preset descriptions.In each
resourcesPresetrow, update the phrasing from:… This is ignored if resources is set (resources is recommended for production).to:
… Ignored if resources are set (resources are recommended for production).This ensures correct plural agreement and improves conciseness.
Example diff:
-| Set container resources according to one common preset (allowed values: none, nano, micro, small, medium, large, xlarge, 2xlarge). This is ignored if resources is set (resources is recommended for production). +| Set container resources according to one common preset (allowed values: none, nano, micro, small, medium, large, xlarge, 2xlarge). Ignored if resources are set (resources are recommended for production).🧰 Tools
🪛 LanguageTool
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |small...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |micro...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |micro...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |micro...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
64-65: Ensure consistent order ofresourcesPresetandresourcesentries.The
controlPlane.controllerManagertable currently listsresourcesbeforeresourcesPreset, whereas other components list the preset first. Swap these two lines for consistency.Proposed diff:
-| `controlPlane.controllerManager.resources` | Resources | `{}` | -| `controlPlane.controllerManager.resourcesPreset` | Set container resources … Ignored if resources are set | `micro` | +| `controlPlane.controllerManager.resourcesPreset` | Set container resources … Ignored if resources are set | `micro` | +| `controlPlane.controllerManager.resources` | Resources | `{}` |🧰 Tools
🪛 LanguageTool
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |micro...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
packages/apps/kubernetes/values.yaml (1)
118-119: Define defaults forcontrollerManagerresources.Defaults for the controller manager (
resourcesPreset: "micro"and an emptyresourcesmap) are correct. Consider swapping the two## @paramcomments above to mirror the key order (resourcesPresetbeforeresources) to avoid confusion.packages/apps/kubernetes/values.schema.json (1)
21-30: Fix grammatical agreement inresourcesPresetdescriptions.Each
resourcesPresetdescription currently reads:… This is ignored if resources is set (resources is recommended for production).For grammatical correctness and clarity, update to:
… Ignored if resources is set (resources are recommended for production).Apply this change consistently across
apiServer,controllerManager,scheduler, andkonnectivity.server.Example diff for
apiServer:"resourcesPreset": { "type": "string", - "description": "Set container resources according to one common preset (allowed values: none, nano, micro, small, medium, large, xlarge, 2xlarge). This is ignored if resources is set (resources is recommended for production).", + "description": "Set container resources according to one common preset (allowed values: none, nano, micro, small, medium, large, xlarge, 2xlarge). Ignored if resources is set (resources are recommended for production).", "default": "small" },Also applies to: 41-45, 51-60, 69-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/apps/kubernetes/README.md(1 hunks)packages/apps/kubernetes/templates/cluster.yaml(2 hunks)packages/apps/kubernetes/values.schema.json(1 hunks)packages/apps/kubernetes/values.yaml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/apps/kubernetes/README.md
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | small ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | micro ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | micro ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | micro ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (9)
packages/apps/kubernetes/templates/cluster.yaml (4)
113-117: UpdateapiServerresource references tocontrolPlane.The Helm template now correctly sources
.Values.controlPlane.apiServerinstead of the old.Values.kamajiControlPlane. The conditional logic and indentation remain unchanged.
119-123: UpdatecontrollerManagerresource references tocontrolPlane.The
.Values.controlPlane.controllerManagerpath is now used consistently for both direct resources and presets. Resource handling logic is preserved.
125-129: Updateschedulerresource references tocontrolPlane.The scheduler block correctly pulls from
.Values.controlPlane.scheduler.resourcesand.Values.controlPlane.scheduler.resourcesPreset. No functional changes.
138-142: Updatekonnectivity.serverresource references tocontrolPlane.The nested
addons.konnectivity.serverblock has been flattened to.Values.controlPlane.konnectivity.server. Both direct and preset‐based resource logic remain intact.packages/apps/kubernetes/README.md (1)
58-60: Rename section header to reflect unified naming.The new "Kubernetes control plane configuration" header accurately replaces the previous
kamajiControlPlaneterminology withcontrolPlane, matching the refactor across the chart.packages/apps/kubernetes/values.yaml (4)
94-99: Introduce newcontrolPlanesection.The previous
kamajiControlPlaneblock has been removed and replaced with a top-levelcontrolPlanesection (includingreplicas: 2). This aligns the values file with the updated Helm templates and documentation.
112-113: Define defaults forapiServerresources.Defaults for the API server (
resourcesPreset: "small"and an emptyresourcesmap) are correctly set and match the schema.
124-125: Define defaults forschedulerresources.The scheduler’s default settings (
resourcesPreset: "micro",resources: {}) align with both the schema and chart logic.
131-132: Define defaults forkonnectivityserver resources.The flattened
controlPlane.konnectivity.serversection (resourcesPreset: "micro",resources: {}) correctly reflects the new hierarchical structure.
8525df6 to
de2d71d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/apps/kubernetes/Makefile(1 hunks)packages/apps/kubernetes/README.md(1 hunks)packages/apps/kubernetes/templates/cluster.yaml(2 hunks)packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml(1 hunks)packages/apps/kubernetes/values.schema.json(1 hunks)packages/apps/kubernetes/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/apps/kubernetes/Makefile
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/apps/kubernetes/templates/cluster.yaml
- packages/apps/kubernetes/values.schema.json
- packages/apps/kubernetes/values.yaml
🧰 Additional context used
🪛 LanguageTool
packages/apps/kubernetes/README.md
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | small ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | micro ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | micro ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | micro ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Test
- GitHub Check: pre-commit
🔇 Additional comments (7)
packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml (1)
33-36: Inline override values block correctly replaces external Secret
Thewith .Values.addons.certManager.valuesOverrideblock cleanly embeds user-supplied overrides directly undervalues:. UsingtoYaml . | nindent 4ensures proper indentation and aligns with the other HelmRelease templates in this PR.packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml (1)
33-36: Consistent inline values override for GPU operator
Switching from a Secret-backedvaluesFromto an inlinevalues:block viawithandtoYaml . | nindent 4is implemented correctly, matching the pattern established across other addon HelmReleases.packages/apps/kubernetes/README.md (4)
58-59: Skip trivial heading rename
The header change from the old section title to### Kubernetes control plane configurationis clear and self-explanatory; no further review needed here.
62-63: ApproveapiServerparameter rename and ordering
The entries forcontrolPlane.apiServer.resourcesPresetfollowed bycontrolPlane.apiServer.resourcescorrectly reflect the new prefix and the intended ordering (preset first, then explicit resources).🧰 Tools
🪛 LanguageTool
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |small...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
66-67: Approveschedulerparameter rename and ordering
The entries forcontrolPlane.scheduler.resourcesPresetfollowed bycontrolPlane.scheduler.resourcescorrectly use the new prefix and the preset-then-resources ordering.🧰 Tools
🪛 LanguageTool
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |micro...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
68-69: Approvekonnectivity.serverparameter rename and ordering
The entries forcontrolPlane.konnectivity.server.resourcesPresetfollowed bycontrolPlane.konnectivity.server.resourcesare correctly updated and ordered.🧰 Tools
🪛 LanguageTool
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |micro...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml (1)
56-59:Details
❓ Verification inconclusive
Validate override injection and mapping structure
The new inline
values:block replaces the external Secret reference, but please verify:
- Flux’s HelmRelease CRD supports multiple
values:entries underspecand appends them rather than clobbering the first map.- The override payload (
.Values.addons.verticalPodAutoscaler.valuesOverride) is structured with the same root keys (e.g.vertical-pod-autoscaler:) so it merges into the default chart values.- Rendering the final manifest yields valid YAML and the overrides apply as expected.
🏁 Script executed:
#!/usr/bin/env bash # Verify HelmRelease rendering with an override value helm template my-release path/to/chart \ --set addons.verticalPodAutoscaler.valuesOverride.vertical-pod-autoscaler.recommender.extraArgs.container-name-label=foo \ | yq e '.spec.values' -Length of output: 371
Validate override injection and mapping structure
Please manually verify the following in
packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml(around lines 56–59):
- Flux’s HelmRelease CRD merges multiple
spec.valuesentries rather than overwriting earlier blocks.- The override object under
.Values.addons.verticalPodAutoscaler.valuesOverrideuses the same root key (vertical-pod-autoscaler:) so it merges into the chart’s defaults.- Rendering the final manifest (for example with
helm template … --set addons.verticalPodAutoscaler.valuesOverride.vertical-pod-autoscaler…) produces valid YAML and shows both default and override values under.spec.values.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
de2d71d to
9f5230a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/apps/kubernetes/README.md (1)
64-65: Ensure consistent ordering ofresourcesPresetbeforeresourcesfor controllerManagerThe
controlPlane.controllerManagersection currently listsresourcesbeforeresourcesPreset, which is inconsistent with the pattern used for other components. Swap these two lines so thatresourcesPresetprecedesresources.- | `controlPlane.controllerManager.resources` | Resources | `{}` | - | `controlPlane.controllerManager.resourcesPreset` | Set container resources according to one common preset (allowed values: none, nano, micro, small, medium, large, xlarge, 2xlarge). This is ignored if resources is set (resources is recommended for production). | `micro` | + | `controlPlane.controllerManager.resourcesPreset` | Set container resources according to one common preset (allowed values: none, nano, micro, small, medium, large, xlarge, 2xlarge). This is ignored if resources is set (resources is recommended for production). | `micro` | + | `controlPlane.controllerManager.resources` | Resources | `{}` |🧰 Tools
🪛 LanguageTool
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |micro...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🧹 Nitpick comments (2)
packages/apps/kubernetes/README.md (1)
62-69: Improve description grammar forresourcesPresetentriesThe phrase "This is ignored if resources is set (resources is recommended for production)" has a verb agreement issue and can be more concise. Consider rewriting to "This setting is ignored if resources are set (resources are recommended for production)" for clarity and consistency.
- This is ignored if resources is set (resources is recommended for production) + This setting is ignored if resources are set (resources are recommended for production)🧰 Tools
🪛 LanguageTool
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |small...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |micro...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |micro...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). |micro...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml (1)
76-79: Embed fluxcd override values inline
This block replaces the external Secret reference and inlines.Values.addons.fluxcd.valuesOverrideunderspec.values. Confirm that omittingspec.valuesentirely when no overrides are provided is acceptable and that the chart’s built-in defaults are applied correctly.Consider adding a
defaultFluxcdValuessnippet
For consistency with other addons, you might introduce a default values snippet for FluxCD and merge it with overrides (usingmerge+toYaml | nindent).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/apps/kubernetes/Makefile(1 hunks)packages/apps/kubernetes/README.md(1 hunks)packages/apps/kubernetes/templates/cluster.yaml(2 hunks)packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml(2 hunks)packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml(2 hunks)packages/apps/kubernetes/values.schema.json(1 hunks)packages/apps/kubernetes/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/apps/kubernetes/templates/cluster.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/apps/kubernetes/Makefile
- packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml
- packages/apps/kubernetes/values.yaml
- packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml
- packages/apps/kubernetes/values.schema.json
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🪛 LanguageTool
packages/apps/kubernetes/README.md
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~62-~62: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | small ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~65-~65: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | micro ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~66-~66: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | micro ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... 2xlarge). This is ignored if resources is set (resources is recommended for produ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~68-~68: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... ignored if resources is set (resources is recommended for production). | micro ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (4)
packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml (2)
4-24: DefinedefaultVPAValuessnippet for DRY Helm values
The new snippet centralizes the VPA configuration, making it easier to maintain and update default settings. Usinglookupto resolve the namespace and templating the tenant address is a clean approach.
59-59: Merge default VPA values with user overrides
Inlines the merged default settings and.Values.addons.verticalPodAutoscaler.valuesOverride, removing the external Secret indirection. This simplifies overrides and is consistent with the other addon templates.packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml (2)
1-11: Review default ingress-nginx values snippet
ThedefaultIngressValuessnippet encapsulates common overrides (fullnameOverride, controller settings,nodeSelector). Please verify these defaults match the chart’svalues.yaml, as any drift in chart parameters could lead to misconfigurations.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
46-46: Inline merge of default and override ingress-nginx values
Replaces the previousvaluesFromSecret pattern with a direct merge ofdefaultIngressValuesand.Values.addons.ingressNginx.valuesOverride. This streamlines value management and aligns with the refactoring of other HelmReleases.
| {{- define "cozystack.defaultVPAValues" -}} | ||
| {{- $myNS := lookup "v1" "Namespace" "" .Release.Namespace }} | ||
| {{- $targetTenant := index $myNS.metadata.annotations "namespace.cozystack.io/monitoring" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing monitoring annotation gracefully
Indexing directly into metadata.annotations will cause a template error if the annotation is not present. Introduce a fallback using the default function so clusters without the annotation don’t break the chart rendering.
- {{- $targetTenant := index $myNS.metadata.annotations "namespace.cozystack.io/monitoring" }}
+ {{- $targetTenant := default .Release.Namespace (index $myNS.metadata.annotations "namespace.cozystack.io/monitoring") }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- define "cozystack.defaultVPAValues" -}} | |
| {{- $myNS := lookup "v1" "Namespace" "" .Release.Namespace }} | |
| {{- $targetTenant := index $myNS.metadata.annotations "namespace.cozystack.io/monitoring" }} | |
| {{- define "cozystack.defaultVPAValues" -}} | |
| {{- $myNS := lookup "v1" "Namespace" "" .Release.Namespace }} | |
| {{- $targetTenant := default .Release.Namespace (index $myNS.metadata.annotations "namespace.cozystack.io/monitoring") }} |
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
Summary by CodeRabbit
kamajiControlPlanetocontrolPlaneacross configuration files.