WPB-24481 helm make proxy deployment optional#5161
Conversation
|
I have run the |
There was a problem hiding this comment.
Pull request overview
Makes the proxy Helm chart a first-class (and tag-controlled) dependency of the wire-server umbrella chart, so operators can enable/disable proxy via tags.proxy.
Changes:
- Add a new
charts/proxychart and wire it intocharts/wire-server/requirements.yamlunder theproxytag. - Update CI/integration values generation to include
tags.proxy(and adjust helper scripts to account forproxybeing a standalone chart again). - Add release notes describing the upgrade consideration around
tags.proxy.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Fix render-ci-manifest to run the values render step and manifest render step as separate commands. |
| hack/helm_vars/wire-server/values.yaml.gotmpl | Enable tags.proxy in CI-rendered values. |
| hack/bin/set-wire-server-image-version.sh | Include the new proxy chart when bumping image tags in .local/charts. |
| charts/wire-server/templates/proxy/configmap.yaml | Remove the in-umbrella proxy ConfigMap template (now owned by the proxy subchart). |
| charts/wire-server/requirements.yaml | Add proxy as a dependency controlled by the proxy tag. |
| charts/proxy/Chart.yaml | Introduce the new proxy chart definition. |
| charts/proxy/.helmignore | Add Helm ignore patterns for the new chart. |
| charts/proxy/values.yaml | Add default values for the new proxy chart. |
| charts/proxy/templates/deployment.yaml | Create proxy Deployment using top-level values (no .Values.proxy.* nesting). |
| charts/proxy/templates/service.yaml | Create proxy Service using top-level values. |
| charts/proxy/templates/secret.yaml | Create proxy Secret from chart values. |
| charts/proxy/templates/configmap.yaml | Create proxy ConfigMap using chart config values. |
| charts/proxy/templates/poddisruptionbudget.yaml | Add PDB for proxy based on replicaCount. |
| charts/proxy/templates/servicemonitor.yaml | Add optional ServiceMonitor controlled by metrics.serviceMonitor.enabled. |
| charts/proxy/templates/_helpers.tpl | Add helper to conditionally include securityContext based on Kubernetes version. |
| changelog.d/0-release-notes/WPB-24481 | Document upgrade behavior regarding tags.proxy. |
Comments suppressed due to low confidence (1)
charts/proxy/templates/secret.yaml:12
proxy.configis rendered from.Values.secrets.proxy_configwithout any guard/required, but the chart's default values don't definesecrets/proxy_config. This makeshelm template/helm lintfail with a low-signalb64encerror when the value is omitted. Consider adding arequiredwith a helpful message (and/or afor_helm_lintingpattern likecharts/nginz/templates/secret.yaml) and documenting the expectedsecrets.proxy_configincharts/proxy/values.yaml.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Sven Tennie <sven.tennie@wire.com>
supersven
left a comment
There was a problem hiding this comment.
Overall, this should be fine as it's only re-introducing previously existing code.
I hope, we haven't forgotten anything. 🤞 (However, at least staging deployment/testing should reveal that.)
The comments I've added are probably worth considering, though.
|
can't we leave the proxy chart part of wire-server and just make it disabled by default? |
https://wearezeta.atlassian.net/browse/WPB-24481
Checklist
changelog.d