Conversation
|
lgtm |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a new Helm chart for Kueue v0.14.3, a Kubernetes job queueing system, to the incubator directory. The chart provides a complete deployment configuration for the Kueue controller manager with comprehensive RBAC, webhooks, CRDs, and visibility components.
Key changes:
- Complete Helm chart structure with templates for deployment, services, RBAC, webhooks, and CRDs
- Configuration files including values.yaml with detailed controller manager settings
- Test suite for validating deployment configurations
- Documentation (README) with installation and configuration instructions
Reviewed Changes
Copilot reviewed 74 out of 75 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| incubator/kueue/values.yaml | Defines default configuration values including controller manager settings, probe configurations, and service definitions |
| incubator/kueue/templates/manager/manager.yaml | Main deployment template for the Kueue controller manager |
| incubator/kueue/templates/webhook/* | Webhook service and configuration templates |
| incubator/kueue/templates/rbac/* | RBAC resources including roles, bindings, and service accounts |
| incubator/kueue/templates/crd/* | Custom Resource Definitions for Kueue resources |
| incubator/kueue/tests/manager_test.yaml | Helm unit tests for manager deployment |
| incubator/kueue/Chart.yaml | Chart metadata and version information |
| incubator/kueue/README.md | Installation and configuration documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| path: /healthz | ||
| port: 8081 | ||
| initialDelaySeconds: {{ .Values.controllerManager.livenessProbe.initialDelaySeconds }} | ||
| periodSeconds: {{ .Values.controllerManager.livenessProbe.initialDelaySeconds }} |
There was a problem hiding this comment.
The periodSeconds field is incorrectly using initialDelaySeconds value. It should use periodSeconds instead. This should be {{ .Values.controllerManager.livenessProbe.periodSeconds }}
| path: /readyz | ||
| port: 8081 | ||
| initialDelaySeconds: {{ .Values.controllerManager.readinessProbe.initialDelaySeconds }} | ||
| periodSeconds: {{ .Values.controllerManager.readinessProbe.initialDelaySeconds }} |
There was a problem hiding this comment.
The periodSeconds field is incorrectly using initialDelaySeconds value. It should use periodSeconds instead. This should be {{ .Values.controllerManager.readinessProbe.periodSeconds }}
| periodSeconds: {{ .Values.controllerManager.readinessProbe.initialDelaySeconds }} | |
| periodSeconds: {{ .Values.controllerManager.readinessProbe.periodSeconds }} |
| # -- Override the resource name | ||
| fullnameOverride: "" |
There was a problem hiding this comment.
The comment on line 7 is misleading. It says 'Override the resource name' but should say 'Override the full resource name' to differentiate it from nameOverride which overrides just the chart name.
add kueue v0.14.3