Helm Chart Updates and Ingress Support #3484
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @minebaev, thank you for submitting a PR! We will respond as soon as possible.
|
Hi @minebaev - thanks for the great PR! I'll look to merge this within the next week. |
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades the Helm chart to v0.3.0, restructures value mappings, and adds full Ingress support.
- Bump chart and app versions
- Refactor values (image, labels, podLabels, env, resources) and update service/deployment templates
- Introduce a new Ingress template with annotations, TLS, and host/path routing
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| containers/helm/values.yaml | Moved deployment fields to top-level (image, labels, podLabels, env, resources) and added ingress config |
| containers/helm/templates/service.yaml | Removed hardcoded namespace and switched to top-level .Values.labels |
| containers/helm/templates/ingress.yaml | Added new Ingress manifest with annotations, className, TLS, and rules |
| containers/helm/templates/deployment.yaml | Updated selector/label handling, added env, .Values.service.port, toYaml probes, and resources |
| containers/helm/Chart.yaml | Bumped version to 0.3.0 and appVersion to "1.58.0" |
Comments suppressed due to low confidence (5)
containers/helm/templates/ingress.yaml:29
- The
rules:block is rendered unconditionally. If.Values.ingress.hostsis empty, this produces an invalid Ingress YAML. Wrap therulessection in anifcheck or ensure a default host entry is provided.
rules:
containers/helm/templates/ingress.yaml:12
- [nitpick] Using
with .Values.ingress.annotationsalways emits anannotations:block, even if the map is empty. Consider switching toifto only render the block when annotations are supplied.
annotations:
containers/helm/values.yaml:16
- [nitpick] The
envmap is introduced without examples. Add a comment or sample block showing how to define key/value pairs so users know how to supply environment variables.
env: {}
containers/helm/templates/service.yaml:7
- [nitpick] The template now ranges over
.Values.labelsinstead of.Values.deployment.labelswithout a fallback. Consider retaining a fallback or documenting this breaking change so existing users can migrate smoothly.
{{- range $k, $v := .Values.labels }}
containers/helm/values.yaml:43
- New Ingress functionality is added but lacks CI/test coverage. Consider adding Helm chart tests or CI validation (e.g., using
helm unittest) to verify ingress configurations.
ingress:
|
@MindTooth If you have some time, could you help review or bless this PR? |
There was a problem hiding this comment.
@pglombardo should maybe also look at bumping the chart when new versions of pwpush are released? Not sure of the proper mapping of versions.
|
|
||
| ingress: | ||
| enabled: false | ||
| className: "nginx" |
There was a problem hiding this comment.
Maybe not hardcode the ingressclass. Usually you have a default one set by ingressclass.kubernetes.io/is-default-class: true.
| podLabels: | ||
| app: "pwpush" | ||
|
|
||
| env: {} |
There was a problem hiding this comment.
I like the approach from Mend Renovate CE that they put application login under its own section. These envs will explicitly be for pwpush?
Description
This MR introduces several improvements to the Helm chart, including version bumps, structural changes, and the addition of Ingress support
Version Updates
0.2.0to0.3.01.37.9to1.58.0Deployment Improvements
deployment.labelsto top-levellabelspodLabelsconfigurationenvvaluesdeployment.containerPorttoservice.porttoYamlNew Features
pwpush.example.comConfiguration Changes
Related Issue
#1527
Type of Change
Checklist
rake test)