Conversation
|
@tuteng:Thanks for your contribution. For this PR, do we need to update docs? |
github-actions
bot
added
the
doc-info-missing
There was a problem hiding this comment.
Pull request overview
This pull request adds support for configuring Istio revision values in the sn-platform and sn-platform-slim Helm charts. The revision field allows users to specify which Istio control plane revision to use for sidecar injection, which is essential for supporting Istio canary upgrades and multi-revision deployments.
Changes:
- Added a new
revisionfield to theistioconfiguration section in both charts' values.yaml files - Applied the revision configuration to broker, bookkeeper, and zookeeper cluster templates where Istio is enabled
- Maintained consistency with existing conditional patterns for optional Istio features
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/sn-platform/values.yaml | Added revision field with documentation to istio configuration |
| charts/sn-platform/templates/broker/broker-cluster.yaml | Added conditional revision field to broker's istio configuration |
| charts/sn-platform-slim/values.yaml | Added revision field with documentation to istio configuration |
| charts/sn-platform-slim/templates/broker/broker-cluster.yaml | Added conditional revision field to broker's istio configuration |
| charts/sn-platform-slim/templates/bookkeeper/bookkeeper-cluster.yaml | Added conditional revision field to bookkeeper's istio configuration |
| charts/sn-platform-slim/templates/zookeeper/zookeeper-cluster.yaml | Added conditional revision field to zookeeper's istio configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # prometheus and grafana will not be Istio injected if mergeMetrics is enabled | ||
| mergeMetrics: false | ||
| # Istio revision to use for control plane injection | ||
| # When set, this revision will be applied to broker, bookkeeper, and zookeeper components |
There was a problem hiding this comment.
The comment states that the revision will be applied to "broker, bookkeeper, and zookeeper components", but the proxy component also has an istio configuration section and would benefit from revision support for consistency. Consider updating the proxy-cluster.yaml template to include the revision field in its istio section, and update this comment to reflect all components that support the revision field.
| # When set, this revision will be applied to broker, bookkeeper, and zookeeper components | |
| # When set, this revision will be applied to broker, bookkeeper, zookeeper, and proxy components |
| # Leave empty for default behavior (sidecar mode) | ||
| dataplaneMode: "" | ||
| # Istio revision to use for control plane injection | ||
| # When set, this revision will be applied to broker, bookkeeper, and zookeeper components |
There was a problem hiding this comment.
The comment states that the revision will be applied to "broker, bookkeeper, and zookeeper components", but the proxy component also has an istio configuration section and would benefit from revision support for consistency. Consider updating the proxy-cluster.yaml template to include the revision field in its istio section, and update this comment to reflect all components that support the revision field.
| # When set, this revision will be applied to broker, bookkeeper, and zookeeper components | |
| # When set, this revision will be applied to all components that support Istio revision configuration |
(If this PR fixes a github issue, please add
Fixes #<xyz>.)Fixes #
(or if this PR is one task of a github issue, please add
Master Issue: #<xyz>to link to the master issue.)Master Issue: #
Motivation
Explain here the context, and why you're making that change. What is the problem you're trying to solve.
Modifications
Describe the modifications you've done.
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Documentation
Check the box below.
Need to update docs?
doc-required(If you need help on updating docs, create a doc issue)
no-need-doc(Please explain why)
doc(If this PR contains doc changes)