Skip to content

Sync kubex charts from automation-controller main @ 60e9c5d#112

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

Sync kubex charts from automation-controller main @ 60e9c5d#112
kubexautomation[bot] wants to merge 1 commit into
masterfrom
sync/kubex-charts/main-60e9c5d

Conversation

@kubexautomation
Copy link
Copy Markdown
Contributor

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

Source commit:

  • 60e9c5d

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 13:55
properties:
conditions:
description: |-
conditions represent the current state of the StaticPolicy resource.
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: Copy-paste error — conditions description says "StaticPolicy resource" instead of "PodAffinity resource".

Why it matters: This is the machine-readable description surfaced by kubectl describe and tooling dashboards. Pointing to the wrong resource type is confusing for operators debugging PodAffinity objects and indicates the file was templated from StaticPolicy without a full search-and-replace.

Suggested fix:

conditions represent the current state of the PodAffinity resource.

spec:
group: rightsizing.kubex.ai
names:
kind: PodAffinity
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: The CRD kind: PodAffinity (in the rightsizing.kubex.ai group) shadows the well-known core Kubernetes type PodAffinity used inside pod.spec.affinity.

Why it matters: Many Kubernetes tools, admission webhooks, RBAC policy engines, and documentation generators match on Kind alone. This naming collision can cause confusion in audit logs, RBAC policies, and operator tooling. It also makes code and manifests harder to reason about since readers must always qualify the group to distinguish the two.

Suggested fix: Consider a disambiguating name such as NodeAffinityPolicy, WorkloadAffinityPolicy, or PodAffinityPolicy that clearly belongs to the rightsizing.kubex.ai domain and does not clash with the built-in type.

type: string
type: array
required:
- 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: High]

Issue: namespaceSelector is required inside spec.scope, but the CRD is scope: Cluster (line 15). A cluster-scoped resource has no owning namespace, so requiring a namespace filter on every instance may be surprising.

Why it matters: Users creating a truly cluster-wide PodAffinity policy must still supply a namespaceSelector, which is non-obvious. If the intent is to always restrict by namespace, a comment in the description would help; if the intent is optionality for cluster-wide coverage, namespaceSelector should be removed from required.

Suggested fix: Either add an explanatory description clarifying that namespaceSelector is mandatory even for the cluster-scoped resource, or remove it from required and handle the nil case in the controller.

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: weight has minimum: 0 but no maximum constraint.

Why it matters: Without an upper bound, a user could set an arbitrarily large value (e.g. MaxInt32 = 2147483647). While the controller may handle this gracefully, documenting an expected ceiling (e.g. maximum: 1000) makes the tie-breaking semantics explicit and prevents accidental misuse of the priority scheme.

Suggested fix: Add a maximum that matches the controller's intended range, e.g.:

minimum: 0
maximum: 1000

@kubexautomation
Copy link
Copy Markdown
Contributor Author

CONTENT OF THIS REVIEW IS AI GENERATED

Overall Assessment

This PR introduces a new PodAffinity CRD (rightsizing.kubex.ai/v1alpha1) and registers it consistently across the RBAC ClusterRole (get/list/watch/create/delete/patch/update, finalizers, status sub-resources). The mechanical wiring is correct and complete. There is one documentation copy-paste bug, a potentially confusing type name, and two schema design observations worth considering before GA promotion.

Risk level: Low (the controller-generated scaffold is sound; issues are documentation quality and API ergonomics rather than functional breakage)


Critical issues

None


Major issues

  • Copy-paste residue in status.conditions description (rightsizing.kubex.ai_podaffinities.yaml, line 173): The conditions block description reads "current state of the StaticPolicy resource" — the wrong resource name. This is surfaced verbatim by kubectl describe and Kubernetes dashboards and should read PodAffinity resource.

  • Kind name collision with core Kubernetes PodAffinity (line 11): The CRD registers kind: PodAffinity in the custom rightsizing.kubex.ai group, which shadows the built-in PodAffinity struct used inside pod.spec.affinity. Many RBAC auditing tools, admission webhooks, and operator UIs match on Kind alone. A more domain-specific name (NodeAffinityPolicy, WorkloadAffinityPolicy, PodAffinityPolicy) would eliminate the ambiguity. This is an API compatibility concern — changing the kind name post-GA requires a version bump and migration.


Minor issues

  • namespaceSelector required on a cluster-scoped resource (line 154): The CRD is scope: Cluster, yet spec.scope.namespaceSelector is in required. Users wanting a truly cluster-wide policy must still supply namespace filter values, which is non-obvious. Either document the intent explicitly or make namespaceSelector optional.

  • No maximum constraint on weight (line 162): weight only has minimum: 0. Adding a documented ceiling (e.g. maximum: 1000) makes the priority tie-breaking range explicit and prevents accidental large values.


DRY / consistency observations

  • The status.conditions block (structure, validations, x-kubernetes-list-map-keys) is identical across multiple CRDs in this chart. Consider whether a shared _helpers.tpl anchor or a generator-level base schema could reduce future copy-paste drift.

Suggested next steps

  1. Fix the StaticPolicyPodAffinity copy-paste in the conditions description before merge.
  2. Evaluate the kind: PodAffinity naming before this version is promoted beyond v1alpha1, as renames require API migrations.
  3. Clarify or relax the namespaceSelector required constraint and document the intended cluster-wide vs namespace-scoped usage pattern.

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