Conversation
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Initial Helm charts for openshift
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Include helm charts to install Kruize on minikube
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Included helm chart tests & values specific to cluster type
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Add workflow to release Kruize helm chart to Github container registry
Add github workflows to run unit tests and helm lint
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Updated templates & tests for Kruize UI pod to deployment
Reviewer's GuideIntroduces a fully structured Kruize Helm chart (including values for default, OpenShift, and Minikube), adds corresponding Kubernetes templates, and wires up CI workflows for linting, unit testing, and publishing the chart to GHCR. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The NetworkPolicy template hardcodes the prometheus pod label selector and port (app.kubernetes.io/name: prometheus, port 9090); consider making the target labels and port configurable via values so it can work with clusters that use different labels or ports for Prometheus.
- The cronjobs.yaml template derives LOGGING_LEVEL, DB_CONFIG_FILE, and KRUIZE_CONFIG_FILE by scanning kruize.env, which can become fragile if env var names change; you might want to drive those values directly from dedicated config keys (or helper functions) instead of coupling the cronjob behavior to the env array layout.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The NetworkPolicy template hardcodes the prometheus pod label selector and port (app.kubernetes.io/name: prometheus, port 9090); consider making the target labels and port configurable via values so it can work with clusters that use different labels or ports for Prometheus.
- The cronjobs.yaml template derives LOGGING_LEVEL, DB_CONFIG_FILE, and KRUIZE_CONFIG_FILE by scanning kruize.env, which can become fragile if env var names change; you might want to drive those values directly from dedicated config keys (or helper functions) instead of coupling the cronjob behavior to the env array layout.
## Individual Comments
### Comment 1
<location path="charts/kruize/templates/role.yaml" line_range="8-10" />
<code_context>
+metadata:
+ name: {{ $fullName }}-recommendation-updater
+rules:
+ - apiGroups:
+ - ""
+ resources:
+ - pods
+ - customresourcedefinitions
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid listing `customresourcedefinitions` under the core (`""`) API group to prevent role creation failures.
In this rule, `customresourcedefinitions` is incorrectly listed under `apiGroups: ["""]`; CRDs only exist in `apiextensions.k8s.io`, which you already cover in the next rule. This mismatch can cause ClusterRole creation to fail. Please remove `customresourcedefinitions` from this rule and rely on the `apiextensions.k8s.io` rule instead.
</issue_to_address>
### Comment 2
<location path="charts/kruize/templates/cronjobs.yaml" line_range="26-27" />
<code_context>
+metadata:
+ name: {{ .Release.Name }}-create-partition
+ namespace: {{ $namespace }}
+spec:
+ schedule: {{ default "0 0 25 * *" .Values.cronJob.createSchedule | quote }} # Run on 25th of every month at midnight
+ jobTemplate:
+ spec:
</code_context>
<issue_to_address>
**suggestion:** Both CronJobs share the same `createSchedule` value, making it impossible to independently configure delete schedule.
Since both the create- and delete-partition CronJobs read from `.Values.cronJob.createSchedule`, their schedules are unintentionally coupled and retention can’t be tuned separately. Please introduce a dedicated value (e.g., `.Values.cronJob.deleteSchedule`) for the delete CronJob so its schedule can be configured independently, while keeping a sensible default aligned with the create schedule.
Suggested implementation:
```
schedule: {{ default "0 0 25 * *" .Values.cronJob.createSchedule | quote }} # Run on 25th of every month at midnight
```
```
name: {{ .Release.Name }}-delete-partition
namespace: {{ $namespace }}
spec:
# Default precedence:
# 1) .Values.cronJob.deleteSchedule (delete-specific override)
# 2) .Values.cronJob.createSchedule (align with create schedule by default)
# 3) "0 0 25 * *" (midnight on the 25th of every month)
schedule: {{ default (default "0 0 25 * *" .Values.cronJob.createSchedule) .Values.cronJob.deleteSchedule | quote }}
```
1. In `values.yaml` (or the appropriate values file), introduce a new value under `cronJob`:
```yaml
cronJob:
createSchedule: "0 0 25 * *"
# Optional, defaults to `createSchedule` when unset
deleteSchedule: ""
```
Leaving `deleteSchedule` empty (or omitting it) ensures it inherits the `createSchedule` value by default.
2. Update any documentation or Helm chart README to mention `cronJob.deleteSchedule` as a configurable value for the delete-partition CronJob.
</issue_to_address>
### Comment 3
<location path="charts/kruize/values.yaml" line_range="129-133" />
<code_context>
+ - ReadWriteMany
+ ## @param db.user User for Kruize DB container
+ user: admin
+ ## @param db.password User for Kruize DB container
+ password: admin
+ ## @param db.adminUser Admin User for Kruize DB container
+ adminUser: admin
+ ## @param db.adminPassword Admin password for Kruize DB container
+ adminPassword: admin
+ ## @param db.name Name of the Kruize DB
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Storing database credentials in plain text values and ConfigMaps is a security risk; consider using Kubernetes Secrets.
These fields are defined directly in `values.yaml` and rendered into `configmap_kruize.yaml` (`dbconfigjson`), so credentials are stored in plaintext and likely end up in source control. Please load them from Kubernetes Secrets (e.g., via `envFrom`/`valueFrom` or mounted files) and keep any chart defaults non-sensitive or limited to local/demo use only.
Suggested implementation:
```
## @param db.credentialsSecretName Name of the Kubernetes Secret containing DB user credentials
## The Secret is expected to provide:
## - user: database username
## - password: database password
## If empty, you must create and specify a Secret name before deploying in non-demo environments.
credentialsSecretName: ""
## @param db.adminCredentialsSecretName Name of the Kubernetes Secret containing DB admin credentials
## The Secret is expected to provide:
## - adminUser: admin database username
## - adminPassword: admin database password
## If empty, you must create and specify a Secret name before deploying in non-demo environments.
adminCredentialsSecretName: ""
```
To fully implement the change and avoid storing credentials in plaintext:
1. In `charts/kruize/templates/configmap_kruize.yaml` (or wherever `dbconfigjson` is rendered), stop inlining `db.user`, `db.password`, `db.adminUser`, and `db.adminPassword` into the ConfigMap. Instead, remove those fields from the JSON or mark them as being loaded from environment variables.
2. In the Deployment/StatefulSet template(s) for the Kruize DB container:
- Add `env` entries that use `valueFrom.secretKeyRef` referencing `.Values.db.credentialsSecretName` and `.Values.db.adminCredentialsSecretName` for `user`, `password`, `adminUser`, and `adminPassword` as needed.
- Ensure the keys in the Kubernetes Secrets match the names documented in `values.yaml` (`user`, `password`, `adminUser`, `adminPassword`).
3. Optionally, for local/demo use, you may provide an example Secret manifest (e.g., `examples/kruize-db-secret.yaml`) instead of embedding credentials in `values.yaml`.
</issue_to_address>
### Comment 4
<location path="charts/kruize/README.md" line_range="105" />
<code_context>
+| `kruize.config.monitoringEndPoint` | Monitoring endpoint | `prometheus-k8s` |
+| `kruize.config.saveToDB` | Enable saving data to database | `true` |
+| `kruize.config.dbDriver` | Database driver | `jdbc:postgresql://` |
+| `kruize.config.plots` | Enable plots generation | `true` |
+| `kruize.config.isROSEnabled` | Enable ROS (Resource Optimization Service) | `false` |
+| `kruize.config.local` | Run in local mode | `true` |
</code_context>
<issue_to_address>
**nitpick (typo):** Consider rephrasing "plots generation" to "plot generation" for better grammar.
Alternatively, you could use "Enable generation of plots," which is also grammatically correct and clear.
```suggestion
| `kruize.config.plots` | Enable plot generation | `true` |
```
</issue_to_address>
### Comment 5
<location path="charts/kruize/README.md" line_range="145" />
<code_context>
+| `kruize.config.datasource` | Array of datasource configurations | `[]` (empty, platform-specific) |
+
+**Note:** Datasource configuration is platform-specific:
+- **OpenShift**: Configured in `values-openshift.yaml` with prometheus-k8s and thanos-querier in openshift-monitoring namespace
+- **Minikube**: Configured in `values-minikube.yaml` with prometheus-k8s in monitoring namespace
+- **Generic Kubernetes**: Empty by default, configure based on your monitoring setup
</code_context>
<issue_to_address>
**nitpick (typo):** Consider adding "the" before "openshift-monitoring namespace" for grammatical clarity.
The current wording is fine, but updating it to "in the openshift-monitoring namespace" will read more naturally.
```suggestion
- **OpenShift**: Configured in `values-openshift.yaml` with prometheus-k8s and thanos-querier in the openshift-monitoring namespace
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+8
to
+10
| - apiGroups: | ||
| - "" | ||
| resources: |
There was a problem hiding this comment.
issue (bug_risk): Avoid listing customresourcedefinitions under the core ("") API group to prevent role creation failures.
In this rule, customresourcedefinitions is incorrectly listed under apiGroups: ["""]; CRDs only exist in apiextensions.k8s.io, which you already cover in the next rule. This mismatch can cause ClusterRole creation to fail. Please remove customresourcedefinitions from this rule and rely on the apiextensions.k8s.io rule instead.
Comment on lines
+26
to
+27
| spec: | ||
| schedule: {{ default "0 0 25 * *" .Values.cronJob.createSchedule | quote }} # Run on 25th of every month at midnight |
There was a problem hiding this comment.
suggestion: Both CronJobs share the same createSchedule value, making it impossible to independently configure delete schedule.
Since both the create- and delete-partition CronJobs read from .Values.cronJob.createSchedule, their schedules are unintentionally coupled and retention can’t be tuned separately. Please introduce a dedicated value (e.g., .Values.cronJob.deleteSchedule) for the delete CronJob so its schedule can be configured independently, while keeping a sensible default aligned with the create schedule.
Suggested implementation:
schedule: {{ default "0 0 25 * *" .Values.cronJob.createSchedule | quote }} # Run on 25th of every month at midnight
name: {{ .Release.Name }}-delete-partition
namespace: {{ $namespace }}
spec:
# Default precedence:
# 1) .Values.cronJob.deleteSchedule (delete-specific override)
# 2) .Values.cronJob.createSchedule (align with create schedule by default)
# 3) "0 0 25 * *" (midnight on the 25th of every month)
schedule: {{ default (default "0 0 25 * *" .Values.cronJob.createSchedule) .Values.cronJob.deleteSchedule | quote }}
- In
values.yaml(or the appropriate values file), introduce a new value undercronJob:LeavingcronJob: createSchedule: "0 0 25 * *" # Optional, defaults to `createSchedule` when unset deleteSchedule: ""
deleteScheduleempty (or omitting it) ensures it inherits thecreateSchedulevalue by default. - Update any documentation or Helm chart README to mention
cronJob.deleteScheduleas a configurable value for the delete-partition CronJob.
Comment on lines
+129
to
+133
| ## @param db.password User for Kruize DB container | ||
| password: admin | ||
| ## @param db.adminUser Admin User for Kruize DB container | ||
| adminUser: admin | ||
| ## @param db.adminPassword Admin password for Kruize DB container |
There was a problem hiding this comment.
🚨 suggestion (security): Storing database credentials in plain text values and ConfigMaps is a security risk; consider using Kubernetes Secrets.
These fields are defined directly in values.yaml and rendered into configmap_kruize.yaml (dbconfigjson), so credentials are stored in plaintext and likely end up in source control. Please load them from Kubernetes Secrets (e.g., via envFrom/valueFrom or mounted files) and keep any chart defaults non-sensitive or limited to local/demo use only.
Suggested implementation:
## @param db.credentialsSecretName Name of the Kubernetes Secret containing DB user credentials
## The Secret is expected to provide:
## - user: database username
## - password: database password
## If empty, you must create and specify a Secret name before deploying in non-demo environments.
credentialsSecretName: ""
## @param db.adminCredentialsSecretName Name of the Kubernetes Secret containing DB admin credentials
## The Secret is expected to provide:
## - adminUser: admin database username
## - adminPassword: admin database password
## If empty, you must create and specify a Secret name before deploying in non-demo environments.
adminCredentialsSecretName: ""
To fully implement the change and avoid storing credentials in plaintext:
- In
charts/kruize/templates/configmap_kruize.yaml(or whereverdbconfigjsonis rendered), stop inliningdb.user,db.password,db.adminUser, anddb.adminPasswordinto the ConfigMap. Instead, remove those fields from the JSON or mark them as being loaded from environment variables. - In the Deployment/StatefulSet template(s) for the Kruize DB container:
- Add
enventries that usevalueFrom.secretKeyRefreferencing.Values.db.credentialsSecretNameand.Values.db.adminCredentialsSecretNameforuser,password,adminUser, andadminPasswordas needed. - Ensure the keys in the Kubernetes Secrets match the names documented in
values.yaml(user,password,adminUser,adminPassword).
- Add
- Optionally, for local/demo use, you may provide an example Secret manifest (e.g.,
examples/kruize-db-secret.yaml) instead of embedding credentials invalues.yaml.
| | `kruize.config.monitoringEndPoint` | Monitoring endpoint | `prometheus-k8s` | | ||
| | `kruize.config.saveToDB` | Enable saving data to database | `true` | | ||
| | `kruize.config.dbDriver` | Database driver | `jdbc:postgresql://` | | ||
| | `kruize.config.plots` | Enable plots generation | `true` | |
There was a problem hiding this comment.
nitpick (typo): Consider rephrasing "plots generation" to "plot generation" for better grammar.
Alternatively, you could use "Enable generation of plots," which is also grammatically correct and clear.
Suggested change
| | `kruize.config.plots` | Enable plots generation | `true` | | |
| | `kruize.config.plots` | Enable plot generation | `true` | |
| | `kruize.config.datasource` | Array of datasource configurations | `[]` (empty, platform-specific) | | ||
|
|
||
| **Note:** Datasource configuration is platform-specific: | ||
| - **OpenShift**: Configured in `values-openshift.yaml` with prometheus-k8s and thanos-querier in openshift-monitoring namespace |
There was a problem hiding this comment.
nitpick (typo): Consider adding "the" before "openshift-monitoring namespace" for grammatical clarity.
The current wording is fine, but updating it to "in the openshift-monitoring namespace" will read more naturally.
Suggested change
| - **OpenShift**: Configured in `values-openshift.yaml` with prometheus-k8s and thanos-querier in openshift-monitoring namespace | |
| - **OpenShift**: Configured in `values-openshift.yaml` with prometheus-k8s and thanos-querier in the openshift-monitoring namespace |
Signed-off-by: Chandrakala Subramanyam <csubrama@ibm.com>
Add kruize icon image referenced in chart yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a new Helm chart for deploying Kruize with configurable DB and UI components, along with platform-specific values and CI workflows.
New Features:
Build:
CI:
Documentation:
Tests: