Included helm chart tests & values specific to cluster type#6
Open
chandrams wants to merge 11 commits intokruize:mvp_demofrom
Open
Included helm chart tests & values specific to cluster type#6chandrams wants to merge 11 commits intokruize:mvp_demofrom
chandrams wants to merge 11 commits intokruize:mvp_demofrom
Conversation
Reviewer's GuideAdds a fully structured Kruize Helm chart with platform-specific values for OpenShift and Minikube, introduces templated manifests (deployments, services, configmaps, storage, RBAC, cronjobs, monitoring, UI proxy), and backs them with an extensive helm-unittest test suite and JSON schema/README documentation. Flow diagram for Helm values and platform-specific configurationgraph LR
User[User] -->|chooses_cluster_type| ClusterType
ClusterType -->|OpenShift| ValuesOpenshift[values_openshift_yaml]
ClusterType -->|Minikube| ValuesMinikube[values_minikube_yaml]
ClusterType -->|Generic_Kubernetes| ValuesDefault[values_yaml]
ValuesDefault --> HelmEngine[Helm_template_engine]
ValuesOpenshift --> HelmEngine
ValuesMinikube --> HelmEngine
HelmEngine -->|renders| Templates[Helm_templates]
Templates -->|produces| K8sManifests[Kubernetes_manifests]
K8sManifests -->|applied_to| Cluster[Kubernetes_cluster]
subgraph Examples_of_overrides
ValuesDefault --> DefaultDatasource[datasource_empty]
ValuesOpenshift --> OSMetricSources[prometheus_and_thanos_in_openshift_monitoring]
ValuesMinikube --> MiniMetricSource[prometheus_in_monitoring_namespace]
ValuesDefault --> DefaultRBAC[rbac_create_true]
ValuesMinikube --> MiniRBAC[rbac_create_false]
end
OSMetricSources --> KruizeConfig[ConfigMap_kruize_config]
MiniMetricSource --> KruizeConfig
DefaultDatasource --> KruizeConfig
KruizeConfig --> KruizePod[Running_kruize_pod]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
chandrams
changed the title
![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 2 issues, and left some high level feedback:
- In
kruize_db_deployment.yamlthelabelsblock under the pod template has a mis‑indentedinclude "kruize.selectorLabels"line (it’s indented with a tab and doesn’t line up underlabels:), which will produce invalid YAML at render time; align it with spaces like in the other templates. - In
cronjobs.yamlboth the create‑partition and delete‑partition CronJobs use the samecronJob.createSchedulevalue; if you expect these to run on different cadences it would be clearer to introduce a separatecronJob.deleteSchedule(or similar) value and wire that into the delete CronJob. - The NetworkPolicy in
network_policy.yamlhardcodesapp.kubernetes.io/name: prometheusand port9090, while the monitoring configuration is otherwise driven bykruize.config.datasource/monitoringvalues; consider wiring the selector/port to values (or documenting the assumption) so the chart can better support non‑Prometheus or differently labeled monitoring stacks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `kruize_db_deployment.yaml` the `labels` block under the pod template has a mis‑indented `include "kruize.selectorLabels"` line (it’s indented with a tab and doesn’t line up under `labels:`), which will produce invalid YAML at render time; align it with spaces like in the other templates.
- In `cronjobs.yaml` both the create‑partition and delete‑partition CronJobs use the same `cronJob.createSchedule` value; if you expect these to run on different cadences it would be clearer to introduce a separate `cronJob.deleteSchedule` (or similar) value and wire that into the delete CronJob.
- The NetworkPolicy in `network_policy.yaml` hardcodes `app.kubernetes.io/name: prometheus` and port `9090`, while the monitoring configuration is otherwise driven by `kruize.config.datasource`/`monitoring` values; consider wiring the selector/port to values (or documenting the assumption) so the chart can better support non‑Prometheus or differently labeled monitoring stacks.
## Individual Comments
### Comment 1
<location path="charts/kruize/templates/kruize_db_deployment.yaml" line_range="5-14" />
<code_context>
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: {{ $fullName }}-config
+ namespace: {{ .Release.Namespace }}
</code_context>
<issue_to_address>
**issue (bug_risk):** Mixed tab/space indentation under `metadata.labels` will likely break YAML parsing.
The line with `{{- include "kruize.selectorLabels" . | nindent 8 }}` appears to use a tab instead of spaces. Mixed indentation can cause Kubernetes to reject the manifest. Please replace the tab with 8 spaces (and any other tabs in this block) so it correctly aligns under `labels:`.
</issue_to_address>
### Comment 2
<location path="charts/kruize/templates/network_policy.yaml" line_range="12-14" />
<code_context>
+ labels:
+ {{- include "kruize.labels" . | nindent 4 }}
+spec:
+ podSelector:
+ matchLabels:
+ app.kubernetes.io/name: prometheus
+ policyTypes:
+ - Ingress
</code_context>
<issue_to_address>
**suggestion (bug_risk):** NetworkPolicy selects Prometheus pods by a hard-coded label that may not match common deployments.
The `podSelector` only matches pods labeled `app.kubernetes.io/name: prometheus`, but many deployments (kube-prometheus-stack, OpenShift, etc.) use different labels (e.g., `prometheus-k8s`). If the labels don’t match, this policy won’t apply to any Prometheus pods. Consider making the selector configurable via values or aligning it with the labels used by your Prometheus datasources.
Suggested implementation:
```
spec:
podSelector:
matchLabels:
{{- if .Values.prometheus.podSelector.matchLabels }}
{{ toYaml .Values.prometheus.podSelector.matchLabels | nindent 6 }}
{{- else }}
app.kubernetes.io/name: prometheus
{{- end }}
policyTypes:
- Ingress
```
1. In `charts/kruize/values.yaml`, add a configurable section, for example:
```yaml
prometheus:
podSelector:
# matchLabels to select Prometheus pods for the NetworkPolicy.
# Example for kube-prometheus-stack:
# matchLabels:
# app.kubernetes.io/name: kube-prometheus-stack-prometheus
matchLabels: {}
```
2. Update the chart README / values documentation to explain how to set `prometheus.podSelector.matchLabels` to match the labels used by the target Prometheus deployment (e.g., kube-prometheus-stack, OpenShift `prometheus-k8s`, etc.).
</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
+12
to
+14
| podSelector: | ||
| matchLabels: | ||
| app.kubernetes.io/name: prometheus |
There was a problem hiding this comment.
suggestion (bug_risk): NetworkPolicy selects Prometheus pods by a hard-coded label that may not match common deployments.
The podSelector only matches pods labeled app.kubernetes.io/name: prometheus, but many deployments (kube-prometheus-stack, OpenShift, etc.) use different labels (e.g., prometheus-k8s). If the labels don’t match, this policy won’t apply to any Prometheus pods. Consider making the selector configurable via values or aligning it with the labels used by your Prometheus datasources.
Suggested implementation:
spec:
podSelector:
matchLabels:
{{- if .Values.prometheus.podSelector.matchLabels }}
{{ toYaml .Values.prometheus.podSelector.matchLabels | nindent 6 }}
{{- else }}
app.kubernetes.io/name: prometheus
{{- end }}
policyTypes:
- Ingress
- In
charts/kruize/values.yaml, add a configurable section, for example:prometheus: podSelector: # matchLabels to select Prometheus pods for the NetworkPolicy. # Example for kube-prometheus-stack: # matchLabels: # app.kubernetes.io/name: kube-prometheus-stack-prometheus matchLabels: {}
- Update the chart README / values documentation to explain how to set
prometheus.podSelector.matchLabelsto match the labels used by the target Prometheus deployment (e.g., kube-prometheus-stack, OpenShiftprometheus-k8s, etc.).
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>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
Signed-off-by: Chandrakala Subramanyam <csubrama@redhat.com>
chandrams
force-pushed
the
helm_charts_tests
branch
from
aa1f3c5 to
e43e9ff
Compare
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.
Changes in this PR
This is on top of #4
Summary by Sourcery
Add a fully featured Kruize Helm chart with environment-specific configuration and introduce comprehensive unit tests for chart resources across Kubernetes, OpenShift, and Minikube.
New Features:
Enhancements:
Documentation:
Tests: