feature/add-multi-dc-for-pg #655
Conversation
dosubot
bot
added
the
size:M
|
Caution Review failedThe pull request is closed. WalkthroughThe changes update the PostgreSQL chart version from 0.8.0 to 0.9.0 and modify the version reference in the versions map to use a fixed commit hash for 0.8.0 while adding an entry for 0.9.0. Additionally, several YAML template files across different packages now include a conditional block that retrieves and processes the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Templating Engine
participant CM as ConfigMap ("cozystack-scheduling")
participant YF as YAML Formatter
participant SR as Spec Renderer
T->>CM: Lookup ConfigMap in cozy-system
alt ConfigMap exists
CM-->>T: Return ConfigMap data
T->>T: Check for "topologyConfig"
alt topologyConfig exists
T->>YF: Convert YAML to formatted string
YF-->>T: Return indented YAML
T->>SR: Include formatted topologyConfig in spec
else No topologyConfig
T->>SR: Render spec without topologyConfig
end
else ConfigMap not found
T->>SR: Render spec without topologyConfig
end
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
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 (
|
![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 (3)
packages/apps/postgres/values.schema.json (1)
105-109: Schema definition could be more specificThe
itemsfield is currently empty, which means any array items would be validated against an empty schema. Consider enhancing this to properly validate the structure of Kubernetes topology spread constraints."topologySpreadConstraints": { "type": "array", "description": "The topologySpreadConstraints", "default": [], - "items": {} + "items": { + "type": "object", + "properties": { + "maxSkew": { + "type": "integer", + "description": "The degree to which pods may be unevenly distributed" + }, + "topologyKey": { + "type": "string", + "description": "The key of node labels" + }, + "whenUnsatisfiable": { + "type": "string", + "description": "How to deal with a pod if it doesn't satisfy the spread constraint", + "enum": ["DoNotSchedule", "ScheduleAnyway"] + } + } + }packages/apps/postgres/README.md (2)
63-63: Typo in Backup Parameter DescriptionThere is a typo in the description for
backup.enabled("Enable pereiodic backups"). Please correct "pereiodic" to "periodic" for clarity.
71-71: Reconsider Parameter Placement and DescriptionThe newly added
topologySpreadConstraintsparameter is inserted under the "Backup parameters" section. Given its purpose for dynamic deployment configuration, consider either:
- Moving it to a more appropriate section (e.g., "Deployment Parameters"), or
- Updating its description to clarify its role within backup or broader deployment settings.
This will help users better understand its function and context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/apps/postgres/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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pre-commit
🔇 Additional comments (2)
packages/apps/versions_map (1)
72-73: Version updates look good!The changes properly version the postgres package, fixing version 0.8.0 to a specific commit hash and correctly setting 0.9.0 to track HEAD.
packages/apps/postgres/values.yaml (1)
80-85: Well-structured configuration with helpful examplesThe addition of the
topologySpreadConstraintsparameter with commented examples provides clear guidance for users who want to configure topology spread constraints.
| {{ if .Values.topologySpreadConstraints }} | ||
| topologySpreadConstraints: | ||
| {{- toYaml .Values.topologySpreadConstraints | nindent 4 }} | ||
| {{- end }} |
There was a problem hiding this comment.
Fix YAML indentation to ensure proper parsing
The static analysis tool reports a syntax error on line 10. While the templating logic is correct, there might be an indentation issue with how the topologySpreadConstraints key is placed relative to other spec fields.
spec:
instances: {{ .Values.replicas }}
enableSuperuserAccess: true
- {{ if .Values.topologySpreadConstraints }}
- topologySpreadConstraints:
- {{- toYaml .Values.topologySpreadConstraints | nindent 4 }}
- {{- end }}
+ {{- if .Values.topologySpreadConstraints }}
+ topologySpreadConstraints:
+ {{- toYaml .Values.topologySpreadConstraints | nindent 4 }}
+ {{- end }}
postgresql:
parameters:The issue is with the whitespace handling in the templating. Using {{- instead of {{ at the beginning of the if statement will properly trim whitespace and prevent YAML parsing errors.
📝 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.
| {{ if .Values.topologySpreadConstraints }} | |
| topologySpreadConstraints: | |
| {{- toYaml .Values.topologySpreadConstraints | nindent 4 }} | |
| {{- end }} | |
| spec: | |
| instances: {{ .Values.replicas }} | |
| enableSuperuserAccess: true | |
| {{- if .Values.topologySpreadConstraints }} | |
| topologySpreadConstraints: | |
| {{- toYaml .Values.topologySpreadConstraints | nindent 4 }} | |
| {{- end }} | |
| postgresql: | |
| parameters: |
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: syntax error: could not find expected ':'
(syntax)
klinch0
force-pushed
the
feature/add-multi-dc
branch
from
fd19f81 to
4cce138
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/extra/monitoring/templates/alerta/alerta-db.yaml (1)
8-14: Dynamic ConfigMap Block and YAML Indentation
The new block that fetches and processestopologyConfigvia thelookupfunction is implemented consistently with similar changes elsewhere. However, please verify that the output YAML fragment (viafromYaml | toYaml | nindent 2) integrates correctly under the parentspecmapping. Note that YAMLlint flagged a syntax error on line 9; this is likely a false positive due to the templating logic.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 9-9: syntax error: could not find expected ':'
(syntax)
packages/system/keycloak/templates/db.yaml (1)
9-15: Conditional TopologyConfig Extraction Block
This conditional block that retrievestopologyConfigfrom thecozystack-schedulingConfigMap mirrors the pattern used in other parts of the PR. Please confirm that the dynamically injected YAML is valid and properly indented within the overall YAML structure. The static analysis error on line 10 may be a templating false positive.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: syntax error: could not find expected ':'
(syntax)
packages/apps/ferretdb/templates/postgres.yaml (1)
9-15: Dynamic Inclusion of Topology Constraints
The conditional block for fetchingtopologyConfigis in line with the broader update. As with similar blocks, ensure that the output YAML fragment (indented bynindent 2) correctly merges into the surrounding configuration. The YAMLlint error on line 10 should be revisited to ensure the final rendered output is valid.packages/extra/monitoring/templates/grafana/db.yaml (1)
9-15: Dynamic Topology Constraints Inclusion
This conditional block for injecting topology constraints from thecozystack-schedulingConfigMap is implemented consistently with similar updates in the PR. Please double-check that the output YAML—formatted withfromYaml | toYaml | nindent 2—merges correctly under the current mapping. The YAMLlint error on line 10 may be a false positive; if it persists, consider wrapping the dynamic content under an explicit key.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: syntax error: could not find expected ':'
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/apps/ferretdb/templates/postgres.yaml(1 hunks)packages/apps/postgres/Chart.yaml(1 hunks)packages/apps/postgres/templates/db.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/extra/monitoring/templates/alerta/alerta-db.yaml(1 hunks)packages/extra/monitoring/templates/grafana/db.yaml(1 hunks)packages/system/keycloak/templates/db.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps/postgres/Chart.yaml
- packages/apps/versions_map
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/extra/monitoring/templates/alerta/alerta-db.yaml
[error] 9-9: syntax error: could not find expected ':'
(syntax)
packages/extra/monitoring/templates/grafana/db.yaml
[error] 10-10: syntax error: could not find expected ':'
(syntax)
packages/system/keycloak/templates/db.yaml
[error] 10-10: syntax error: could not find expected ':'
(syntax)
packages/apps/postgres/templates/db.yaml
[error] 10-10: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (1)
packages/apps/postgres/templates/db.yaml (1)
9-15: YAML Indentation and Dynamic Block Insertion
The topology constraints block injected via the ConfigMap lookup is implemented using the templating functions. However, as noted in previous reviews, ensure that its output—currently indented usingnindent 2—aligns correctly within thespecmapping. YAMLlint reported a potential syntax error on line 10; consider verifying the final YAML structure or wrapping the dynamic content within an explicit key if needed.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: syntax error: could not find expected ':'
(syntax)
2 participants
Summary by CodeRabbit
New Features
Chores