Revert kamaji edge-24.12.1 #610
Conversation
Signed-off-by: Andrei Kvapil <[email protected]>
dosubot
bot
added
the
size:M
WalkthroughThis pull request introduces significant updates to the Kamaji project's configuration and documentation. The changes primarily focus on version updates across multiple files, including Changes
Possibly related PRs
Suggested Labels
Poem
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
🧹 Nitpick comments (1)
packages/system/kamaji/charts/kamaji/README.md (1)
25-25: Consider using formatted link for URL.Replace the bare URL with a formatted Markdown link.
-| https://clastix.github.io/charts | kamaji-etcd | >=0.7.0 | +| [https://clastix.github.io/charts](https://clastix.github.io/charts) | kamaji-etcd | >=0.7.0 |🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
25-25: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/system/kamaji/charts/kamaji/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
packages/system/kamaji/charts/kamaji/Chart.yaml(2 hunks)packages/system/kamaji/charts/kamaji/README.md(3 hunks)packages/system/kamaji/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml(4 hunks)packages/system/kamaji/charts/kamaji/templates/controller.yaml(1 hunks)packages/system/kamaji/charts/kamaji/values.yaml(1 hunks)packages/system/kamaji/images/kamaji/Dockerfile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/system/kamaji/charts/kamaji/values.yaml
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/system/kamaji/charts/kamaji/README.md
25-25: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (7)
packages/system/kamaji/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml (2)
6416-6418: LGTM! Clear documentation improvement for the dataStore field.The updated description provides better clarity about:
- The optional nature of the field
- Its purpose as an override for the default DataStore
- Important warning about migration limitations
6543-6544: Verify the default CIDR values align with Kubernetes best practices.The default values for network CIDRs need verification:
- dnsServiceIPs: 10.96.0.10
- podCidr: 10.244.0.0/16
- serviceCidr: 10.96.0.0/16
Also applies to: 6550-6550, 6558-6559
✅ Verification successful
Default CIDR values are correctly aligned with Kubernetes best practices
The specified CIDR values match the standard Kubernetes defaults and follow networking best practices:
- Service CIDR (10.96.0.0/16) matches kubeadm's default
- Pod CIDR (10.244.0.0/16) is the common default for CNI plugins
- DNS Service IP (10.96.0.10) is appropriately positioned within the service CIDR range
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the CIDR values align with Kubernetes documentation and common practices # Search for any existing CIDR configurations in the codebase echo "Searching for existing CIDR configurations..." rg --type yaml "podCIDR|serviceCIDR|clusterCIDR" -A 2 -B 2 # Search for any documentation about CIDR ranges echo "Searching for CIDR documentation..." fd -t f "README.md" -X grep -l "CIDR"Length of output: 3320
packages/system/kamaji/charts/kamaji/templates/controller.yaml (1)
36-37: Breaking change: Verify the impact of making defaultDatastoreName required.While using the
requiredfunction improves robustness by ensuring the datastore is explicitly configured, this is a breaking change that could affect existing deployments that don't specify.Values.defaultDatastoreName. Since this is part of a revert PR, please clarify:
- What issues led to this revert?
- How should users handle the migration?
- Should this be documented in the upgrade notes?
Let's check for potential impact on existing deployments:
✅ Verification successful
Not a breaking change: defaultDatastoreName has a default value
The change to make
.Values.defaultDatastoreNamerequired is safe because:
- The values.yaml provides a default value:
defaultDatastoreName: default- The kamaji-etcd subchart is pre-configured to use this default value
- The
requiredfunction only triggers if someone explicitly nullifies or removes the valueThe change actually improves robustness by preventing accidental nullification while maintaining backward compatibility through the default value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Helm value files or examples that might be affected echo "Searching for Helm value files..." fd -e yaml -e yml | while read -r file; do if grep -l "defaultDatastoreName" "$file" > /dev/null; then echo "Found in $file:" grep -A 5 "defaultDatastoreName" "$file" fi doneLength of output: 65572
Script:
#!/bin/bash # Search for defaultDatastoreName in kamaji-related files and show content echo "=== Checking values.yaml in kamaji chart ===" if [ -f "packages/system/kamaji/charts/kamaji/values.yaml" ]; then echo "Content of values.yaml:" cat "packages/system/kamaji/charts/kamaji/values.yaml" fi echo -e "\n=== Checking for example configurations ===" find . -type f -name "*.yaml" -o -name "*.md" | while read -r file; do if grep -l "defaultDatastoreName" "$file" > /dev/null; then echo -e "\nFile: $file" grep -A 5 -B 5 "defaultDatastoreName" "$file" fi doneLength of output: 68691
packages/system/kamaji/images/kamaji/Dockerfile (2)
4-4: Version downgrade aligns with revert intention.The VERSION downgrade from edge-24.12.1 to edge-24.9.2 matches the PR's revert purpose.
1-1:⚠️ Potential issueInvalid Go version specified.
The base image
golang:1.23doesn't exist. The latest stable version is 1.21.Apply this diff to fix the Go version:
-FROM golang:1.23 as builder +FROM golang:1.21 as builderLikely invalid or redundant comment.
packages/system/kamaji/charts/kamaji/Chart.yaml (1)
2-2: Version changes inconsistent with revert PR.The version upgrades (appVersion to v1.0.0, version to 2.0.0, and relaxed etcd dependency) seem inconsistent with the PR's intention to revert "kamaji edge-24.12.1". Please verify if these version changes should also be reverted.
Let's verify the previous versions:
Also applies to: 20-20, 24-24
packages/system/kamaji/charts/kamaji/README.md (1)
3-3: Version badges inconsistent with revert PR.The version badges (Version: 2.0.0, AppVersion: v1.0.0) match Chart.yaml but seem inconsistent with the PR's intention to revert "kamaji edge-24.12.1". Please verify if these should also be reverted.
Due to upstream issue: clastix/kamaji#679
Summary by CodeRabbit
New Features
Documentation
Chores