Sync kubex charts from automation-controller main @ 2ed7cc9#113
Sync kubex charts from automation-controller main @ 2ed7cc9#113kubexautomation[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
Overall assessment: This PR introduces a new PodAffinity CRD, supporting RBAC entries, a hardened deployment annotation, and documentation improvements. The structure is clean and the role.yaml additions are consistent. However, both charts downgrade their semver version numbers, which is the most significant concern for chart repository consumers.
Risk level: Medium
Critical issues
None
Major issues
- Chart version downgrade in both
kubex-automation-engineandkubex-crds(Chart.yamlin each):
Both charts move from1.0.1→1.0.0. This violates Helm/OCI chart repository immutability conventions — if1.0.0was ever published, re-publishing under the same version with different content will be silently accepted by some registries and rejected by others, causing unpredictable behaviour for users who already have1.0.1deployed. The correct fix is to bump to1.0.2(or higher). If this is a deliberate reset, the PR body should document it and the registry must be verified to have no prior1.0.0artifact.
Minor issues
-
Hardcoded
pause-until: infiniteannotation (deployment.yamlline 13): Novalues.yamltoggle exists, so users cannot opt out. Recommend exposing viacontrollerManager.pauseRightsizingor a generalized annotations merge. -
Positional JSON Patch index in Troubleshooting docs (
Troubleshooting.mdline 15): Theargs/3index is fragile and will silently patch the wrong field if argument ordering changes. Add a note advising users to verify the index first, or use a value-targeted approach. -
Stale
StaticPolicycopy-paste in CRD description (rightsizing.kubex.ai_podaffinities.yamlline 162): Thestatus.conditionsdescription references "StaticPolicy" instead of "PodAffinity". Surfaced verbatim bykubectl explain. -
Wildcard support not documented in CRD schema (
rightsizing.kubex.ai_podaffinities.yamlline 107): ThenamespaceSelector.valuesdescription does not mention*wildcard support, creating a discoverability gap relative toClusterProactivePolicyandPolicy-Configuration.md. -
kubex-crdsiconfield removed without replacement: Thekubex-automation-enginechart updated its icon URL;kubex-crdssimply removed it. This degrades the chart's visual presentation in Artifact Hub and other Helm UIs.
DRY improvement opportunities
- The
workloadTypesdefault list ([Deployment, StatefulSet, CronJob, Rollout, Job, AnalysisRun, DaemonSet]) appears verbatim in both the newPodAffinityCRD schema and in existing CRD schemas. This is expected in auto-generated CRDs (kubebuilder output), but worth confirming the generator is the single source of truth so lists don't drift.
Suggested next steps
- Resolve the version downgrade — bump both charts to
1.0.2(or confirm a deliberate reset is safe and document it). - Fix the
StaticPolicystale reference in the CRD conditions description. - Add wildcard documentation to the CRD's
namespaceSelector.valuesfield. - Add an
iconback tokubex-crds/Chart.yaml(matching the new URL used inkubex-automation-engine). - Consider exposing the
pause-untilannotation as avalues.yamloption for operator flexibility.
| @@ -2,8 +2,8 @@ apiVersion: v2 | |||
| name: kubex-automation-engine | |||
| description: A Helm chart for deploying kubex-automation-engine for automated workload rightsizing and resource optimization | |||
| type: application | |||
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Major] [Confidence: High]
Issue: Chart version is being downgraded from 1.0.1 → 1.0.0.
Why it matters: Helm uses semver to determine whether an upgrade or a downgrade is happening. Reverting the chart version to a lower number than what is already published/deployed can cause helm upgrade to no-op or behave unexpectedly for users who already have 1.0.1 installed. It also breaks immutability guarantees in chart repositories — if 1.0.0 was already released, pushing a different chart under the same version is a breaking convention violation.
Suggested fix: If this is intentional (e.g., a reset before a new release cycle), document it explicitly in the PR body and ensure the chart repository has no prior 1.0.0 artifact. Otherwise, bump the version to 1.0.2 (or higher) to reflect the new content being added in this PR.
| description: CRDs for Kubex Automation Engine | ||
| icon: https://kubex.ai/wp-content/uploads/kubex-logo-landscape.svg | ||
| version: 1.0.1 | ||
| version: 1.0.0 |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Major] [Confidence: High]
Issue: Chart version is being downgraded from 1.0.1 → 1.0.0, mirroring the same problem in kubex-automation-engine/Chart.yaml. Additionally, the icon field is removed entirely without replacement.
Why it matters: Same semver immutability concern applies here. A chart registry (e.g., OCI or a classic chart museum) that already has 1.0.0 cannot accept a different artifact under the same version. Removing icon also degrades the chart's presentation in Helm UIs (Artifact Hub, Rancher, etc.) without explanation.
Suggested fix: Bump the version to 1.0.2 (or higher). If the icon URL was changed (as in kubex-automation-engine), apply the same replacement icon URL here rather than omitting the field entirely.
| metadata: | ||
| name: {{ include "kubex-automation-engine.fullname" . }} | ||
| namespace: {{ include "kubex-automation-engine.namespace" . }} | ||
| annotations: |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Minor] [Confidence: High]
Issue: The rightsizing.kubex.ai/pause-until: infinite annotation is hardcoded directly in the Deployment template with no way for users to override or disable it via values.yaml.
Why it matters: While the self-automation-prevention intent is sound, hardcoding this annotation means:
- Users cannot opt out if their environment handles this differently.
- Operators who want to allow rightsizing on the controller itself (e.g., in a test environment) have no mechanism to do so.
- The annotation value
"infinite"is a non-standard string — if the controller's annotation semantics ever change (e.g., to a timestamp format), this baked-in value will silently misbehave.
Suggested fix: Either expose this via a values.yaml toggle (e.g., controllerManager.pauseRightsizing: true) that conditionally renders the annotation, or at minimum add a {{- with .Values.controllerManager.annotations }} merge point so users can override deployment-level annotations.
|
|
||
| ```bash | ||
| kubectl -n kubex patch deploy/$(kubectl -n kubex get deploy -l app.kubernetes.io/name=kubex-automation-engine -o jsonpath='{.items[0].metadata.name}') --type='json' -p='[{"op":"replace","path":"/spec/template/spec/containers/0/args/3","value":"--zap-log-level=debug"}]' | ||
| ``` |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Minor] [Confidence: High]
Issue: The kubectl patch commands hardcode the JSON Patch path /spec/template/spec/containers/0/args/3, which assumes --zap-log-level is always the 4th argument (index 3) in the container's args array.
Why it matters: If the chart's deployment.yaml ever reorders arguments, or if a user adds/removes entries via controllerManager.extraArgs, the index 3 will point to the wrong arg, silently patching an unintended field (e.g., overwriting a different flag). This can be hard to diagnose in production incidents.
Suggested fix: Add a warning note such as: "Verify the argument index before running: kubectl -n kubex get deploy <name> -o jsonpath='{.spec.template.spec.containers[0].args}'" Or replace the JSON Patch replace with a jq-based approach that targets the arg by its prefix rather than by positional index.
| weight determines which policy wins when multiple PodAffinity policies match. | ||
| Higher weights take precedence. When weights are equal, older policies win. | ||
| format: int32 | ||
| minimum: 0 |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Minor] [Confidence: Medium]
Issue: The status.conditions description copy-pastes the text "conditions represent the current state of the StaticPolicy resource" — but this is the PodAffinity CRD, not a StaticPolicy.
Why it matters: Stale/incorrect documentation in a CRD's OpenAPI schema is surfaced directly to kubectl explain, API docs generators, and operator SDKs. Users querying kubectl explain podaffinity.status.conditions will receive misleading information.
Suggested fix: Update the description to reference PodAffinity:
description: |-
conditions represent the current state of the PodAffinity resource.
...| type: object | ||
| type: object | ||
| x-kubernetes-map-type: atomic | ||
| namespaceSelector: |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Minor] [Confidence: Medium]
Issue: The CRD's namespaceSelector field is marked required under scope, but the PodAffinity description in Policy-Configuration.md shows scope[].namespaces.values supporting wildcards. However, the CRD schema does not document the wildcard behaviour in the values field description — it only says "values contains the namespace name patterns to match", with no mention of * support.
Why it matters: Users relying solely on kubectl explain or generated API docs will not discover the wildcard feature. It also creates a discoverability gap when comparing the ClusterProactivePolicy reference (which does document wildcards) with the new PodAffinity CRD (which does not).
Suggested fix: Augment the values field description in the CRD schema to explicitly mention wildcard support:
description: |-
values contains the namespace name patterns to match.
Supports shell-style '*' wildcards (e.g. 'prod-*').
This PR was created automatically to sync managed Kubex chart content.
Source commit:
2ed7cc9Managed chart scope:
charts/kubex-automation-enginecharts/kubex-crds