feature/add-workloads - #656
Conversation
WalkthroughThis pull request updates chart version numbers and introduces new Kubernetes resource definitions and role rules across several packages. In addition to bumping chart versions for ClickHouse, FerretDB, Kafka, MySQL, NATS, and RabbitMQ, new rules are added to the dashboard/resourcemap YAML files to grant permissions for a new resource type, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/apps/postgres/README.md (1)
61-71: Enhance Topology Spread Constraints Parameter DescriptionThe new row for
topologySpreadConstraintsis correctly added to the Backup parameters table. However, the description"The topologySpreadConstraints"is a bit generic. Consider expanding it to explain that this parameter controls how pods are distributed across failure domains (e.g., zones) for improved availability. This will help users understand its purpose at a glance.packages/apps/postgres/templates/db.yaml (1)
9-12: Validate YAML Templating and Conditional LogicThe conditional block for
topologySpreadConstraintsis implemented appropriately. Note that templated YAML sections can sometimes trigger linting errors (e.g., the YAMLlint error on line 10) even though the rendered output is valid. It would be good to ensure your linting configuration excludes template syntax or verifies rendered output to avoid false positives.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: syntax error: could not find expected ':'
(syntax)
packages/apps/postgres/values.yaml (1)
79-86: Clarify the Topology Spread Constraints Parameter DocumentationThe addition of the
topologySpreadConstraintsparameter is clear and correctly initialized as an empty list. To improve usability, consider providing a more descriptive explanation in the documentation comment so that users understand its role in governing pod distribution. For example, you might mention that it can be used to control pod skew and specify domain keys for enhanced resilience.packages/apps/postgres/values.schema.json (1)
104-110: Confirm JSON Schema Flexibility for Topology Spread ConstraintsThe new
topologySpreadConstraintsproperty is added to the schema as an array with a default of an empty array, which aligns with the other changes. One suggestion is to consider whether theitemsschema (currently an empty object) can be defined more strictly if the expected constraints structure is known. This may help with validating user input more precisely.packages/apps/ferretdb/templates/workloadmonitor.yaml (1)
1-14: Ensure Consistent Selector Key in Ferretdb WorkloadMonitorWhile the WorkloadMonitor resource for Ferretdb is defined correctly overall, note that the selector key is defined as
app(line 12) instead of theapp.kubernetes.io/instancekey used in other applications. Please verify if this divergence is intentional or if it should be updated for consistency across services.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/clickhouse/templates/workloadmonitor.yaml(1 hunks)packages/apps/ferretdb/Chart.yaml(1 hunks)packages/apps/ferretdb/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/ferretdb/templates/workloadmonitor.yaml(1 hunks)packages/apps/kafka/Chart.yaml(1 hunks)packages/apps/kafka/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/kafka/templates/workloadmonitor.yaml(1 hunks)packages/apps/mysql/Chart.yaml(1 hunks)packages/apps/mysql/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/mysql/templates/workloadmonitor.yaml(1 hunks)packages/apps/nats/Chart.yaml(1 hunks)packages/apps/nats/templates/resourcemap.yaml(1 hunks)packages/apps/nats/templates/workloadmonitor.yaml(1 hunks)packages/apps/postgres/Chart.yaml(1 hunks)packages/apps/postgres/README.md(1 hunks)packages/apps/postgres/templates/db.yaml(1 hunks)packages/apps/postgres/values.schema.json(1 hunks)packages/apps/postgres/values.yaml(1 hunks)packages/apps/rabbitmq/Chart.yaml(1 hunks)packages/apps/rabbitmq/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/rabbitmq/templates/workloadmonitor.yaml(1 hunks)packages/apps/versions_map(4 hunks)
✅ Files skipped from review due to trivial changes (7)
- packages/apps/clickhouse/Chart.yaml
- packages/apps/nats/Chart.yaml
- packages/apps/mysql/Chart.yaml
- packages/apps/ferretdb/Chart.yaml
- packages/apps/postgres/Chart.yaml
- packages/apps/rabbitmq/Chart.yaml
- packages/apps/kafka/Chart.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/apps/postgres/templates/db.yaml
[error] 10-10: syntax error: could not find expected ':'
(syntax)
packages/apps/nats/templates/workloadmonitor.yaml
[error] 12-12: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (12)
packages/apps/mysql/templates/workloadmonitor.yaml (1)
1-14: Approved: MySQL WorkloadMonitor YAML is correctly definedThe new YAML correctly defines the WorkloadMonitor resource for MySQL with proper templating for dynamic values. The fields (replicas, kind, type, selector, and version) follow our expected patterns.
packages/apps/kafka/templates/workloadmonitor.yaml (1)
1-14: Approved: Kafka WorkloadMonitor YAML is validThe resource definition for Kafka is consistent with similar configurations. All necessary fields are present and templated correctly.
packages/apps/nats/templates/workloadmonitor.yaml (1)
1-14: Approved (with minor fix): NATS WorkloadMonitor YAMLApart from the key quoting issue noted above, the overall configuration for the NATS WorkloadMonitor is correct and follows the standard structure.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: syntax error: expected , but found ''
(syntax)
packages/apps/kafka/templates/dashboard-resourcemap.yaml (1)
20-26: Approved: New RBAC Rule for WorkloadMonitors Added CorrectlyThe additional rule granting
get,list, andwatchaccess to theworkloadmonitorsresource in thecozystack.ioAPI group is well defined and integrates appropriately with the existing RBAC settings.packages/apps/rabbitmq/templates/workloadmonitor.yaml (1)
1-14: New WorkloadMonitor Resource for RabbitMQ Looks Well-Structured
The YAML correctly defines the custom resource with appropriate templating (e.g. using{{ $.Release.Name }}and dynamic chart values). Ensure that the value forreplicasis defined in the associated values file and that the intent behind both thekindandtypefields being set to "rabbitmq" is well documented for future maintainers.packages/apps/mysql/templates/dashboard-resourcemap.yaml (1)
21-27: Addition of WorkloadMonitors Rule is Consistent and Correct
The new rule grantingget,list, andwatchaccess toworkloadmonitorsunder thecozystack.ioAPI group is implemented correctly. The dynamic templating via{{ .Release.Name }}ensures the rule is appropriately scoped.packages/apps/clickhouse/templates/workloadmonitor.yaml (1)
1-14: ClickHouse WorkloadMonitor Definition is Clear and Consistent
This new resource definition mirrors the structure of the RabbitMQ variant while appropriately using fields likeclickhouse.altinity.com/chiin the selector for proper association. Consider adding inline comments to explain the purpose of each field for better maintainability.packages/apps/rabbitmq/templates/dashboard-resourcemap.yaml (1)
23-29: WorkloadMonitors Rule Added to RabbitMQ Dashboard Role
The additional rule for accessingworkloadmonitorsis integrated consistently with existing permissions for other resources. The templated resource name and verb list are correct. Ensure that this change is in sync with RBAC requirements across all applications.packages/apps/ferretdb/templates/dashboard-resourcemap.yaml (1)
20-26: Consistent Extension of Access Permissions for WorkloadMonitors
The new RBAC rule for theworkloadmonitorsresource under thecozystack.ioAPI group is correctly added. The use of dynamic templating forresourceNamesensures the rule applies as intended. No issues found.packages/apps/clickhouse/templates/dashboard-resourcemap.yaml (1)
20-26: RBAC permission properly added for WorkloadMonitor resource.The addition of RBAC permissions for the
workloadmonitorsresource in thecozystack.ioAPI group follows the same pattern as existing rules. The permissions are appropriately scoped to only the specific named resource matching the release name, and limited to read-only operations (get, list, watch).packages/apps/nats/templates/resourcemap.yaml (1)
20-26: RBAC permission properly added for WorkloadMonitor resource.The addition of RBAC permissions for the
workloadmonitorsresource in thecozystack.ioAPI group follows the same pattern as existing rules. The permissions are appropriately scoped to only the specific named resource matching the release name, and limited to read-only operations (get, list, watch).packages/apps/versions_map (1)
9-10: Version updates properly implemented across multiple applications.The version updates follow a consistent pattern across all applications:
- Previous latest versions are now locked to specific commit hashes
- New versions are added with HEAD references
This structured approach supports stability for released versions while allowing development to continue.
Also applies to: 16-17, 29-30, 58-59, 64-65, 77-78, 85-86
| | `backup.s3AccessKey` | The access key for S3, used for authentication | `oobaiRus9pah8PhohL1ThaeTa4UVa7gu` | | ||
| | `backup.s3SecretKey` | The secret key for S3, used for authentication | `ju3eum4dekeich9ahM1te8waeGai0oog` | | ||
| | `backup.resticPassword` | The password for Restic backup encryption | `ChaXoveekoh6eigh4siesheeda2quai0` | | ||
| | `topologySpreadConstraints` | The topologySpreadConstraints | `[]` | |
There was a problem hiding this comment.
This is from another PR
| {{ if .Values.topologySpreadConstraints }} | ||
| topologySpreadConstraints: | ||
| {{- toYaml .Values.topologySpreadConstraints | nindent 4 }} | ||
| {{- end }} |
There was a problem hiding this comment.
This is from another PR
| }, | ||
| "topologySpreadConstraints": { | ||
| "type": "array", | ||
| "description": "The topologySpreadConstraints", | ||
| "default": [], | ||
| "items": {} |
There was a problem hiding this comment.
This is from another PR
|
|
||
| ## @param topologySpreadConstraints The topologySpreadConstraints | ||
| topologySpreadConstraints: [] | ||
| # topologySpreadConstraints: | ||
| # - maxSkew: 1 | ||
| # topologyKey: topology.kubernetes.io/zone | ||
| # whenUnsatisfiable: ScheduleAnyway |
There was a problem hiding this comment.
This is from another PR
ef01a32 to
625fc46
Compare
625fc46 to
0793b1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/apps/nats/templates/workloadmonitor.yaml (1)
11-13: 🛠️ Refactor suggestionQuote dotted key in metadata selector
The keyapp.kubernetes.io/instance(line 12) must be enclosed in quotes to avoid YAML parsing errors. This aligns with previous feedback on similar issues.- app.kubernetes.io/instance: {{ $.Release.Name }}-system + "app.kubernetes.io/instance": {{ $.Release.Name }}-system🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: syntax error: expected , but found ''
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/clickhouse/templates/workloadmonitor.yaml(1 hunks)packages/apps/ferretdb/Chart.yaml(1 hunks)packages/apps/ferretdb/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/ferretdb/templates/workloadmonitor.yaml(1 hunks)packages/apps/kafka/Chart.yaml(1 hunks)packages/apps/kafka/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/kafka/templates/workloadmonitor.yaml(1 hunks)packages/apps/mysql/Chart.yaml(1 hunks)packages/apps/mysql/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/mysql/templates/workloadmonitor.yaml(1 hunks)packages/apps/nats/Chart.yaml(1 hunks)packages/apps/nats/templates/resourcemap.yaml(1 hunks)packages/apps/nats/templates/workloadmonitor.yaml(1 hunks)packages/apps/rabbitmq/Chart.yaml(1 hunks)packages/apps/rabbitmq/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/rabbitmq/templates/workloadmonitor.yaml(1 hunks)packages/apps/versions_map(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/apps/mysql/Chart.yaml
- packages/apps/clickhouse/Chart.yaml
- packages/apps/ferretdb/Chart.yaml
- packages/apps/ferretdb/templates/workloadmonitor.yaml
- packages/apps/nats/Chart.yaml
- packages/apps/kafka/Chart.yaml
- packages/apps/mysql/templates/dashboard-resourcemap.yaml
- packages/apps/mysql/templates/workloadmonitor.yaml
- packages/apps/nats/templates/resourcemap.yaml
- packages/apps/rabbitmq/templates/workloadmonitor.yaml
- packages/apps/rabbitmq/templates/dashboard-resourcemap.yaml
- packages/apps/ferretdb/templates/dashboard-resourcemap.yaml
- packages/apps/clickhouse/templates/workloadmonitor.yaml
- packages/apps/clickhouse/templates/dashboard-resourcemap.yaml
- packages/apps/rabbitmq/Chart.yaml
- packages/apps/versions_map
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/apps/kafka/templates/workloadmonitor.yaml
[error] 21-21: syntax error: expected , but found ''
(syntax)
packages/apps/nats/templates/workloadmonitor.yaml
[error] 12-12: syntax error: expected , but found ''
(syntax)
| - apiGroups: | ||
| - cozystack.io | ||
| resources: | ||
| - workloadmonitors | ||
| resourceNames: | ||
| - {{ .Release.Name }} | ||
| - {{ $.Release.Name }}-zookeeper | ||
| verbs: ["get", "list", "watch"] |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Ensure consistent templating in resourceNames
The new rule uses inconsistent templating: line 25 uses {{ .Release.Name }} while line 26 uses {{ $.Release.Name }}-zookeeper. For consistency (and to avoid potential runtime misinterpretations), please use a consistent context—preferably {{ $.Release.Name }}—for both entries.
- resourceNames:
- - {{ .Release.Name }}
- - {{ $.Release.Name }}-zookeeper
+ resourceNames:
+ - {{ $.Release.Name }}
+ - {{ $.Release.Name }}-zookeeperConsistent Templating Required in resourceNames
In the file packages/apps/kafka/templates/dashboard-resourcemap.yaml, the two entries in the resourceNames array use different Helm templating contexts—one uses {{ .Release.Name }} while the other uses {{ $.Release.Name }}-zookeeper. To avoid any potential runtime issues, please update the first entry to use the root context just like the second entry. For example:
- resourceNames:
- - {{ .Release.Name }}
- - {{ $.Release.Name }}-zookeeper
+ resourceNames:
+ - {{ $.Release.Name }}
+ - {{ $.Release.Name }}-zookeeperThis change ensures consistent templating across both resource names.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - apiGroups: | |
| - cozystack.io | |
| resources: | |
| - workloadmonitors | |
| resourceNames: | |
| - {{ .Release.Name }} | |
| - {{ $.Release.Name }}-zookeeper | |
| verbs: ["get", "list", "watch"] | |
| - apiGroups: | |
| - cozystack.io | |
| resources: | |
| - workloadmonitors | |
| resourceNames: | |
| - {{ $.Release.Name }} | |
| - {{ $.Release.Name }}-zookeeper | |
| verbs: ["get", "list", "watch"] |
| metadata: | ||
| name: {{ $.Release.Name }}-zookeeper | ||
| spec: |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Quote dynamic name for the second WorkloadMonitor
YAMLlint reports a syntax error at line 21, likely due to the unquoted dynamic string value {{ $.Release.Name }}-zookeeper in the metadata. Enclose this value in quotes to ensure valid YAML parsing.
- name: {{ $.Release.Name }}-zookeeper
+ name: "{{ $.Release.Name }}-zookeeper"Action Required: Quote Dynamic Value in YAML
YAMLlint is reporting a syntax error because the dynamic string in the name field is not enclosed in quotes. To ensure valid YAML parsing, please update the value as follows:
-
File:
packages/apps/kafka/templates/workloadmonitor.yaml(lines 20–22) -
Change:
- name: {{ $.Release.Name }}-zookeeper + name: "{{ $.Release.Name }}-zookeeper"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| metadata: | |
| name: {{ $.Release.Name }}-zookeeper | |
| spec: | |
| metadata: | |
| name: "{{ $.Release.Name }}-zookeeper" | |
| spec: |
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 21-21: syntax error: expected , but found ''
(syntax)
Summary by CodeRabbit
New Features
Chores