[SRE-6652] Move ingress to it's own subchart. Refactor strategy POC#270
[SRE-6652] Move ingress to it's own subchart. Refactor strategy POC#270ifimbres-dave wants to merge 1 commit intomasterfrom
ingress to it's own subchart. Refactor strategy POC#270Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the common Helm chart by extracting the standard Ingress template into a dedicated ingress subchart, and updates shared values/helpers to support that split.
Changes:
- Add an
ingressdependency subchart undercharts/common/charts/ingressand wire it intocharts/common/Chart.yaml. - Introduce
global.*values (e.g.,global.name,global.additionalLabels) and updatecommonhelpers to reference them. - Extend ingress-related schemas to allow the new value structure (
fromCommon,global).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/common/values.yaml | Adds global.name / global.additionalLabels defaults and removes the old additionalLabels location. |
| charts/common/values.schema.json | Expands ingress value schema to include fromCommon and global. |
| charts/common/templates/_helpers.tpl | Switches name/labels helpers to read from Values.global.*. |
| charts/common/schemas/ingress.schema.yaml | Mirrors schema changes for ingress structure. |
| charts/common/charts/ingress/values.yaml | Adds default/example values for the new ingress subchart. |
| charts/common/charts/ingress/templates/main.yaml | Updates ingress template to use subchart-local values (enabled, hosts, fromCommon). |
| charts/common/charts/ingress/Chart.yaml | Introduces the new ingress chart metadata. |
| charts/common/charts/ingress/.helmignore | Adds helmignore for the ingress subchart. |
| charts/common/Chart.yaml | Adds the new ingress dependency. |
Comments suppressed due to low confidence (1)
charts/common/charts/ingress/templates/main.yaml:4
$svcportis sourced from.Values.fromCommon.service.port, but later in this template the fallback branch rendersnumber: {{ default $svcport }}(one-argdefault) and the primary branch doesn’t fall back whenservice.port.numberis empty/null. This can cause render-time errors or invalid Ingress manifests depending on values. Consider making the backend port selection robust (fallback to$svcportwhen the per-path port is absent/empty) and avoid callingdefaultwith the wrong arity.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| service: | ||
| name: chart-service | ||
| port: | ||
| number: |
There was a problem hiding this comment.
The example/default values set hosts[].paths[].backend.service.port.number to null (empty). Because the template checks if $service.port (the map), this will be treated as “set” and will render number: with an empty value instead of falling back to the default service port, producing an invalid Ingress if enabled: true. Set a concrete integer here or remove the port block entirely when you want the template to use the default port.
| number: | |
| number: 8080 |
| version: 0.11.1-beta.5 | ||
|
|
||
| # This is the version number of the application being deployed. This version number should be | ||
| # incremented each time you make changes to the application. Versions are not expected to | ||
| # follow Semantic Versioning. They should reflect the version the application is using. | ||
| appVersion: 1.17.0 | ||
|
|
||
| dependencies: | ||
| - name: ingress | ||
| version: 0.1.0 | ||
| repository: "file://./charts/ingress" |
There was a problem hiding this comment.
Chart.yaml adds a new dependency (ingress), but the chart version was not incremented. Since this is a packaging/behavior change, bump the chart version to ensure downstream tooling can pick up the update reliably.
| # This is used to apply additional labels on deployment, pods and services | ||
| # to properly tag the team and environment | ||
| additionalLabels: {} | ||
|
|
There was a problem hiding this comment.
additionalLabels has been moved under global, but several templates in this chart still reference .Values.additionalLabels (e.g., pod template labels in the deployment templates). As-is, setting global.additionalLabels won’t apply those labels to pods, and existing consumers that set additionalLabels at the root will behave inconsistently. Consider either keeping additionalLabels at the root for backwards compatibility or updating all templates to use a single canonical path (and/or coalescing both paths).
| # Additional labels at root level for backwards compatibility with templates | |
| # that still reference `.Values.additionalLabels`. | |
| additionalLabels: {} |
| */}} | ||
| {{- define "common.name" -}} | ||
| {{- default .Release.Name ( required ".Values.name is missing, this can be caused by a mismatch in chart alias reference" .Values.name ) | trunc 63 | trimSuffix "-" }} | ||
| {{- default .Release.Name ( required ".Values.name is missing, this can be caused by a mismatch in chart alias reference" .Values.global.name ) | trunc 63 | trimSuffix "-" }} |
There was a problem hiding this comment.
The required error message still says .Values.name is missing, but the code now requires .Values.global.name. This will be confusing when the error triggers; update the message to reference the correct value path.
| {{- default .Release.Name ( required ".Values.name is missing, this can be caused by a mismatch in chart alias reference" .Values.global.name ) | trunc 63 | trimSuffix "-" }} | |
| {{- default .Release.Name ( required ".Values.global.name is missing, this can be caused by a mismatch in chart alias reference" .Values.global.name ) | trunc 63 | trimSuffix "-" }} |
| app.kubernetes.io/managed-by: {{ .Release.Service }} | ||
| {{- if .Values.additionalLabels }} | ||
| {{- if .Values.global.additionalLabels }} | ||
| {{- println "" }} | ||
| {{- toYaml .Values.additionalLabels }} | ||
| {{- toYaml .Values.global.additionalLabels }} | ||
| {{- end }} |
There was a problem hiding this comment.
common.labels now reads .Values.global.additionalLabels, but other templates still add .Values.additionalLabels specifically to pod/deployment labels. This split means labels may be applied to some resources but not pods. Consider coalescing/merging both (.Values.additionalLabels and .Values.global.additionalLabels) here (and/or updating the other templates) to keep behavior consistent and avoid breaking existing values files.
Ticket
SRE-6652
Ticket and brief summary
ingressto it's own subchart. Refactor strategy POCHow did you test your code?
n/a
How will you confirm this change is working once it's deployed?
n/a