-
Notifications
You must be signed in to change notification settings - Fork 424
Helm Chart Updates and Ingress Support #3484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
MindTooth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach from Mend Renovate CE that they put application logic under its own section. These envs will explicitly be for pwpush?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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)