NATs: fix hardcode, add merge and resolve config #514
Conversation
dosubot
bot
added
the
size:M
WalkthroughThe changes in this pull request primarily involve updates to the NATS application configuration files. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/apps/nats/README.md (1)
15-16: Update description forconfig.resolverparameterThe description for
config.resolveris identical toconfig.merge. Consider updating it to better describe its specific purpose for resolver configuration.-| `config.resolver` | Additional configuration to merge into NATS config | `{}` | +| `config.resolver` | Additional resolver configuration to merge | `{}` |packages/apps/nats/values.yaml (1)
36-39: Update description forconfig.resolverparameterThe description should be more specific about the resolver configuration's purpose, similar to the README update.
- ## Allows you to customize NATS server settings by merging resolver configurations. + ## Allows you to customize NATS resolver settings for service discovery.packages/apps/nats/values.schema.json (2)
44-46: Update description for resolver configurationThe description should be more specific about the resolver's purpose, matching the updates in README and values.yaml.
- "description": "Additional configuration to merge into NATS config", + "description": "Additional resolver configuration for service discovery",
20-34: Consider enhancing schema validationThe schema could be improved with:
- Required fields specification for critical parameters
additionalProperties: falseto prevent unexpected configuration"jetstream": { "type": "object", + "required": ["size", "enabled"], + "additionalProperties": false, "properties": { // ... existing properties ... } }, "config": { "type": "object", + "additionalProperties": false, "properties": { // ... existing properties ... } }Also applies to: 35-48
packages/apps/nats/templates/nats.yaml (2)
43-57: Consider simplifying the merge condition.The condition logic is correct, but could be more concise using
andwithhasKey.Consider this alternative:
- {{- if or (gt (len $passwords) 0) (gt (len .Values.config.merge) 0) }} + {{- if or (gt (len $passwords) 0) (and .Values.config.merge (gt (len .Values.config.merge) 0)) }}This maintains the same functionality while being more idiomatic in Helm templates.
69-74: Consider storage requirements documentation.Since Jetstream requires persistent storage, it would be beneficial to document the storage requirements and implications, especially regarding:
- Minimum recommended storage size
- Storage class compatibility
- Backup considerations
- Scaling implications
Consider adding these details to the README.md file to help users make informed decisions about storage configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/apps/nats/Chart.yaml(1 hunks)packages/apps/nats/README.md(1 hunks)packages/apps/nats/templates/nats.yaml(2 hunks)packages/apps/nats/values.schema.json(1 hunks)packages/apps/nats/values.yaml(1 hunks)packages/apps/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/apps/nats/Chart.yaml
🔇 Additional comments (5)
packages/apps/versions_map (1)
55-56: LGTM! Version mapping correctly updated.
The version mapping changes appropriately reflect:
- Assignment of commit hash
b7375f73to version0.3.1 - Addition of new version
0.4.0asHEAD
This aligns with the version bump in Chart.yaml and follows the established version mapping pattern.
packages/apps/nats/values.yaml (1)
19-35: LGTM! Well-structured configuration with clear documentation.
The Jetstream configuration is well-organized with appropriate defaults and helpful comments explaining each parameter.
packages/apps/nats/values.schema.json (1)
19-48: LGTM! Schema properly validates new configuration parameters.
The schema correctly defines types and defaults for all new parameters, maintaining consistency with values.yaml.
packages/apps/nats/templates/nats.yaml (2)
58-60: LGTM! Proper configuration handling.
The resolver configuration is well-implemented with appropriate existence checks and YAML handling.
70-73: Verify Jetstream configuration values.
The Jetstream configuration looks correct, but let's verify that the required values are properly defined in the values file.
✅ Verification successful
Jetstream configuration values are properly defined and documented
The verification confirms that:
- Values are properly defined in
values.yamlwith default size of10Gi - Both
jetstream.enabledandjetstream.sizeare documented in README.md - Schema validation is in place with proper types and descriptions for both properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Jetstream configuration values are properly defined
# Check for jetstream configuration in values.yaml
echo "Checking for Jetstream configuration in values.yaml:"
rg -A 5 'jetstream:' packages/apps/nats/values.yaml
# Check for documentation of these values
echo -e "\nChecking for documentation in README:"
rg -A 5 'jetstream' packages/apps/nats/README.md
# Check schema validation
echo -e "\nChecking schema validation for Jetstream properties:"
rg -A 10 'jetstream' packages/apps/nats/values.schema.json
Length of output: 1428
dosubot
bot
added
size:L
kvaps
changed the title
Summary by CodeRabbit
Release Notes
New Features
jetstream.sizeandjetstream.enabled, enhancing storage and functionality options.config.mergeandconfig.resolver.Bug Fixes
Version Updates
0.3.1to0.4.0.