HYPERFLEET-291| deployment: helm charts template implemented for adapter deployment #14
Conversation
WalkthroughThis pull request expands and restructures the Helm chart and documentation: greatly extends charts/README.md with a parameterized layout and examples; adds a helper template for broker ConfigMap naming; introduces charts/templates/broker-configmap.yaml to optionally create a broker ConfigMap; augments charts/templates/deployment.yaml with many new deployment, pod, container, env, probe, volume, and topology options and conditional broker mounting; adds runtime validation to charts/templates/pdb.yaml; updates charts/values.yaml with many new configurable fields; and removes hyperfleetApiToken and a clusterSecret resource from configs/adapter-config-template.yaml. No application runtime code paths were changed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
charts/templates/broker-configmap.yaml (1)
1-32: Consider validating broker configuration when create is enabled.The template is gated by
.Values.broker.create, but all data fields (subscriptionId, topic, env, yaml) are independently conditional. This could result in an empty ConfigMap ifbroker.create=truebut no data is provided.Consider adding validation to ensure at least one configuration field is set when broker creation is enabled, or document in values.yaml that enabling
broker.createrequires providing broker configuration.Example validation block to add after line 1:
{{- if .Values.broker.create }} +{{- if not (or .Values.broker.subscriptionId .Values.broker.topic .Values.broker.env .Values.broker.yaml) }} +{{- fail "broker.create is enabled but no broker configuration provided. Set at least one of: broker.subscriptionId, broker.topic, broker.env, or broker.yaml" }} +{{- end }} apiVersion: v1 kind: ConfigMapcharts/templates/deployment.yaml (2)
91-91: Consider defining$brokerEnabledonce at the container scope.The variable
$brokerEnabledis defined identically five times throughout the container definition (lines 91, 160, 259, 270, 285). While this works in Helm templates, defining it once at the beginning of the container block would reduce duplication and improve maintainability.Consider moving the variable definition before the container block or consolidating the conditional logic.
172-252: Document probe endpoint requirements in values.yaml.The health probes default to HTTP endpoints (
/healthz,/readyzon porthttp) if enabled without specific configuration. However, as noted in the README, the adapter doesn't expose HTTP endpoints by default.Consider adding a warning comment in values.yaml for each probe that enabling them requires configuring the appropriate endpoint type (httpGet, tcpSocket, or exec) and ensuring the adapter exposes the necessary endpoints.
charts/values.yaml (1)
194-215: Clarify the adapter ConfigMap configuration and empty ConfigMap behavior.Two observations:
Line 204 comment: References "adapter-configmap-template.yaml" which exists in
configs/(as a configuration example), but the actual Helm template ischarts/templates/configmap.yaml. Consider clarifying which file is being referenced or updating the comment to avoid confusion.Empty ConfigMap potential: With
config.enabled: truebutconfig.env: {},config.adapterType: "", andconfig.adapterYaml: "", the chart will create an empty ConfigMap. While not necessarily an error, consider adding a comment noting that at least one configuration field should be provided, or setconfig.enabled: falseby default to match thebroker.create: falsepattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
charts/README.md(2 hunks)charts/templates/_helpers.tpl(1 hunks)charts/templates/broker-configmap.yaml(1 hunks)charts/templates/deployment.yaml(2 hunks)charts/templates/pdb.yaml(1 hunks)charts/values.yaml(5 hunks)configs/adapter-config-template.yaml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
charts/README.md
[style] ~109-~109: Consider replacing this word to strengthen your wording.
Context: ...ult) The adapter is a message consumer and doesn't expose HTTP endpoints by defaul...
(AND_THAT)
🪛 YAMLlint (1.37.1)
charts/templates/broker-configmap.yaml
[warning] 33-33: too many blank lines (1 > 0)
(empty-lines)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/templates/pdb.yaml
[warning] 22-22: too many blank lines (1 > 0)
(empty-lines)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (5)
charts/templates/_helpers.tpl (1)
75-84: LGTM!The broker ConfigMap naming helper follows the same pattern as the existing
configMapNamehelper, providing consistent naming behavior across the chart.charts/README.md (1)
1-288: Excellent documentation!The README provides comprehensive guidance covering:
- All configuration parameters with clear descriptions
- Multiple practical examples for different deployment scenarios
- Environment variable handling
- GCP Workload Identity setup instructions
- Security features and notes
This will greatly help users understand and deploy the Helm chart correctly.
charts/templates/deployment.yaml (1)
259-308: LGTM!The volume and volumeMount configuration is well-structured:
- Properly coordinates volumes with their mounts
- Uses
readOnly: truefor security- Correctly references ConfigMap names via helpers
- Uses
subPathto mount specific files (adapter.yaml, broker.yaml)- Allows for additional custom volumes via
extraVolumesandextraVolumeMountscharts/values.yaml (1)
257-298: LGTM!The configuration values are well-structured with:
- Sensible defaults (disabled probes, disabled PDB, safe rolling updates)
- Clear documentation and examples
- Security-focused defaults (non-root, read-only filesystem, dropped capabilities)
- Flexible environment variable and volume configuration
The values align well with the templates and provide good guidance for users.
configs/adapter-config-template.yaml (1)
246-246: Verify authentication mechanism for HyperFleet API.The template has removed the
hyperfleetApiTokenparameter and the associatedclusterSecretresource. Ensure that authentication for the HyperFleet API is properly configured through an alternative mechanism (e.g., service account tokens, workload identity, or external secret management).
ciaranRoche
left a comment
ciaranRoche
left a comment
There was a problem hiding this comment.
Aside from the coderabbit comment, looks good to me
# Basic template - ✅ Passes
helm template test ./charts/
# With broker config - ✅ Passes
helm template test ./charts/ \
--set broker.create=true \
--set broker.type=googlepubsub \
--set broker.subscriptionId=my-subscription
# With PDB - ✅ Passes
helm template test ./charts/ \
--set podDisruptionBudget.enabled=true \
--set podDisruptionBudget.minAvailable=1
# Lint - ✅ Passes (1 info about missing icon)
helm lint ./charts/
xueli181114
force-pushed
the
HYPERFLEET-291
branch
from
4585a7d to
70bb691
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
charts/templates/pdb.yaml (1)
24-25: Minor: Remove trailing blank line.YAMLlint flags unnecessary blank lines at end of file. While non-blocking, remove for consistency.
charts/templates/broker-configmap.yaml (1)
32-33: Minor: Remove trailing blank line.Consistent with pdb.yaml feedback.
charts/templates/deployment.yaml (2)
259-281: Volume mount logic correctly handles conditional file mounting.ConfigMaps are mounted using
subPathto mount specific files (adapter.yaml, broker.yaml) with read-only access, which is secure and precise. The conditional logic properly checks if mounts are needed before rendering.Minor: Line 270 redefines
$brokerEnabledwhich is already defined at line 259. This is redundant; consider using the existing variable for consistency.
285-308: Volume definitions are well-structured with proper ConfigMap references.Volumes correctly reference ConfigMaps via helper functions and use
itemsto select specific keys, supporting both adapter and broker configurations plus custom volumes.Minor: Similar to line 270, line 285 redefines
$brokerEnabled. This variable is repeatedly redefined at lines 91, 160, 259, and 285 throughout the template. Consider defining it once at a higher scope to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
charts/README.md(2 hunks)charts/templates/_helpers.tpl(1 hunks)charts/templates/broker-configmap.yaml(1 hunks)charts/templates/deployment.yaml(2 hunks)charts/templates/pdb.yaml(1 hunks)charts/values.yaml(5 hunks)configs/adapter-config-template.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- charts/templates/_helpers.tpl
- configs/adapter-config-template.yaml
🧰 Additional context used
🪛 LanguageTool
charts/README.md
[style] ~109-~109: Consider replacing this word to strengthen your wording.
Context: ...ult) The adapter is a message consumer and doesn't expose HTTP endpoints by defaul...
(AND_THAT)
🪛 YAMLlint (1.37.1)
charts/templates/broker-configmap.yaml
[warning] 33-33: too many blank lines (1 > 0)
(empty-lines)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/templates/pdb.yaml
[warning] 25-25: too many blank lines (1 > 0)
(empty-lines)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (19)
charts/templates/pdb.yaml (2)
1-4: Validation logic is correctly implemented.The fail-fast validation ensures that when
podDisruptionBudget.enabled=true, at least one ofminAvailableormaxUnavailableis set, preventing invalid Kubernetes resources from being generated. This addresses the prior review concern.
12-17: Mutual exclusivity enforcement is sound.The conditions properly ensure that only one of
minAvailableormaxUnavailableis rendered, preventing conflicting Kubernetes specifications.charts/templates/broker-configmap.yaml (2)
1-32: Broker ConfigMap template is well-structured and properly conditioned.The template correctly:
- Gates creation with
.Values.broker.create- Uses the
brokerConfigMapNamehelper for consistent naming- Applies quoting to all scalar values for YAML safety
- Conditionally renders each data field
- Handles multiline content (broker.yaml) with proper indentation
Ensure the referenced helper
brokerConfigMapNameis implemented in_helpers.tpland all.Values.broker.*fields are documented in values.yaml.
27-31: YAML indentation and quoting are correct.The
indent 4filter properly handles multiline YAML content, and all scalar values usequotefilter for safety.charts/README.md (4)
1-10: README introduction and prerequisites are clear.The description accurately reflects the adapter's purpose (CloudEvents consumer for HyperFleet provisioning), and prerequisites are complete including GCP Workload Identity.
45-105: Configuration tables are comprehensive and well-documented.All major parameter sections are covered with clear descriptions, defaults, and conditional behaviors. Tables align with the values.yaml structure.
127-245: Examples are practical and progressively demonstrate increasing complexity.The examples cover common scenarios (basic, API, WIF, existing resources, comprehensive values file) and use realistic placeholder values. This guides users through typical deployment patterns.
247-260: Environment variables reference is clear and condition-based.The table correctly documents which variables are set under what conditions. Note that line 260 mentions
broker.envis loaded viaenvFrom, which should be verified against the deployment.yaml broker mounting logic.charts/templates/deployment.yaml (6)
1-27: Deployment metadata is well-structured with new customization options.Lines 7-13 add
deploymentLabelsanddeploymentAnnotationsfor deployment-level customization, and lines 18-27 add strategy, minReadySeconds, and revisionHistoryLimit for controlled rollouts. This improves operational flexibility.
34-36: Checksum annotation correctly triggers pod restarts on adapter config changes.The annotation uses the standard Helm pattern to detect ConfigMap modifications and trigger pod recreation. Note that the checksum only covers the adapter ConfigMap; broker ConfigMap changes will not trigger pod restarts. This may be intentional if broker configuration is decoupled from pod lifecycle, but clarify if this is by design.
91-115: Environment variable logic is comprehensive but properly gated.Lines 91-93 establish conditional variables to determine if environment configuration is needed, and lines 95-115 render environment variables based on multiple sources (direct env, HyperFleet API, adapter/broker config). The logic correctly:
- Uses local variables to avoid re-evaluating complex conditions
- Applies proper conditionals to each environment variable
- References ConfigMap keys safely via ConfigMapKeyRef
Ensure the helper functions
hyperfleet-adapter.configMapNameandhyperfleet-adapter.brokerConfigMapNameare correctly implemented in_helpers.tpl.
140-171: Broker environment mounting logic is well-designed with clear modes.The implementation correctly supports two mutually exclusive patterns:
- Specific keys: When
broker.envKeysis specified, only those keys are mounted via env (lines 146-157)- All keys: When
broker.envKeysis empty andbroker.mountAsEnv=true, the entire ConfigMap is mounted via envFrom (lines 161-171)This design provides flexibility for both precise configuration and bulk loading of environment variables.
172-252: Health probe configuration is flexible and well-parameterized.The probes support multiple strategies (httpGet, tcpSocket, exec) with configurable defaults, and each probe independently respects the
enabledflag. The default timing values (liveness 30s initial delay, readiness 5s, startup 10s) are reasonable. Disabled by default is appropriate since the adapter is a message consumer without HTTP endpoints.
321-324: Topology spread constraints are properly parameterized.The template defers to values configuration, allowing users full control over pod distribution.
charts/values.yaml (5)
1-53: Values file structure is well-organized with comprehensive documentation.The file covers all necessary deployment parameters with clear defaults:
- Image configuration with registry override support
- Deployment strategy with RollingUpdate (maxSurge: 1, maxUnavailable: 0)
- Command/args properly specified for the adapter
- Clear documentation of customization options
All defaults appear appropriate for a production deployment.
54-179: Pod configuration options provide comprehensive customization while maintaining secure defaults.The section covers:
- ServiceAccount with WIF annotation guidance
- Health probes (disabled by default, with examples for future use)
- Lifecycle hooks for graceful shutdown
- Networking (hostNetwork, DNS, DNS config)
- Scheduling (priority, termination grace period, init/sidecar containers, topology spread)
All defaults are production-appropriate for a non-HTTP message consumer.
194-256: Adapter and broker configuration sections are well-documented with sensible defaults.Both sections provide:
- Clear enable/disable controls
- Custom ConfigMap naming options
- Type identifiers for broker routing
- Rich environment variable support
- YAML configuration file alternatives
Defaults are safe: adapter config enabled by default, broker config disabled by default. Comments provide helpful context including RabbitMQ and Google Pub/Sub examples.
One note: Line 204 references "adapter-configmap-template.yaml" which appears to be an external system file. Clarify if this reference is still relevant or should be updated.
257-291: HyperFleet API and environment configuration sections provide necessary flexibility.The sections support:
- HyperFleet API integration with baseUrl and version
- Direct environment variables with valueFrom support
- ConfigMap/Secret bulk loading via envFrom
- Custom volume support for extensions
All defaults are safe and examples are clear.
293-298: Pod Disruption Budget configuration is properly parameterized.The section provides placeholders for
minAvailableandmaxUnavailablewith optionalunhealthyPodEvictionPolicy, disabled by default. This aligns with the runtime validation in pdb.yaml that requires exactly one of minAvailable/maxUnavailable when enabled.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ciaranRoche, xueli181114 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 82c1a33
into
openshift-hyperfleet:main
2 participants
Summary by CodeRabbit
Documentation
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.