Skip to content

Sync kubex charts from automation-controller main @ 2ed7cc9#113

Closed
kubexautomation[bot] wants to merge 1 commit into
masterfrom
sync/kubex-charts/main-2ed7cc9
Closed

Sync kubex charts from automation-controller main @ 2ed7cc9#113
kubexautomation[bot] wants to merge 1 commit into
masterfrom
sync/kubex-charts/main-2ed7cc9

Conversation

@kubexautomation
Copy link
Copy Markdown
Contributor

This PR was created automatically to sync managed Kubex chart content.

Source commit:

  • 2ed7cc9

Managed chart scope:

  • charts/kubex-automation-engine
  • charts/kubex-crds

@kubexautomation kubexautomation Bot requested a review from a team as a code owner May 19, 2026 18:21
Copy link
Copy Markdown
Contributor Author

@kubexautomation kubexautomation Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-engine and kubex-crds (Chart.yaml in each):
    Both charts move from 1.0.11.0.0. This violates Helm/OCI chart repository immutability conventions — if 1.0.0 was 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 have 1.0.1 deployed. The correct fix is to bump to 1.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 prior 1.0.0 artifact.

Minor issues

  • Hardcoded pause-until: infinite annotation (deployment.yaml line 13): No values.yaml toggle exists, so users cannot opt out. Recommend exposing via controllerManager.pauseRightsizing or a generalized annotations merge.

  • Positional JSON Patch index in Troubleshooting docs (Troubleshooting.md line 15): The args/3 index 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 StaticPolicy copy-paste in CRD description (rightsizing.kubex.ai_podaffinities.yaml line 162): The status.conditions description references "StaticPolicy" instead of "PodAffinity". Surfaced verbatim by kubectl explain.

  • Wildcard support not documented in CRD schema (rightsizing.kubex.ai_podaffinities.yaml line 107): The namespaceSelector.values description does not mention * wildcard support, creating a discoverability gap relative to ClusterProactivePolicy and Policy-Configuration.md.

  • kubex-crds icon field removed without replacement: The kubex-automation-engine chart updated its icon URL; kubex-crds simply removed it. This degrades the chart's visual presentation in Artifact Hub and other Helm UIs.


DRY improvement opportunities

  • The workloadTypes default list ([Deployment, StatefulSet, CronJob, Rollout, Job, AnalysisRun, DaemonSet]) appears verbatim in both the new PodAffinity CRD 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

  1. Resolve the version downgrade — bump both charts to 1.0.2 (or confirm a deliberate reset is safe and document it).
  2. Fix the StaticPolicy stale reference in the CRD conditions description.
  3. Add wildcard documentation to the CRD's namespaceSelector.values field.
  4. Add an icon back to kubex-crds/Chart.yaml (matching the new URL used in kubex-automation-engine).
  5. Consider exposing the pause-until annotation as a values.yaml option 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
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONTENT OF THIS REVIEW IS AI GENERATED

[Severity: Major] [Confidence: High]

Issue: Chart version is being downgraded from 1.0.11.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
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONTENT OF THIS REVIEW IS AI GENERATED

[Severity: Major] [Confidence: High]

Issue: Chart version is being downgraded from 1.0.11.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:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Users cannot opt out if their environment handles this differently.
  2. Operators who want to allow rightsizing on the controller itself (e.g., in a test environment) have no mechanism to do so.
  3. 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"}]'
```
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-*').

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant