[feature] kubespan and allowSchedulingOnControlPlanes options #72
Conversation
|
""" WalkthroughThe changes add conditional logic for a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant TalmCLI
participant TemplateEngine
User->>Makefile: Run `make render-template` with optional args
Makefile->>TalmCLI: Verify `talm` executable
TalmCLI-->>Makefile: Confirm executable or error
Makefile->>TemplateEngine: Initialize workspace with `cozystack` preset
TemplateEngine->>TemplateEngine: Render controlplane.yaml and worker.yaml with flags
TemplateEngine-->>Makefile: Output rendered YAML files
Makefile-->>User: Notify completion and cleanup
Estimated code review effort2 (~20 minutes) 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @lb0o, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the configurability of the Helm charts by introducing new options for kubespan support and making the allowSchedulingOnControlPlanes setting dynamic. These changes allow for more flexible and environment-specific deployments, particularly for network configurations and control plane node behavior.
Highlights
- New Configuration Options: Introduced a new top-level Helm value,
kubespan, to control the enablement of kubespan-related network configurations and cluster discovery settings within themachine.networkandcluster.discoverysections of the generated configuration. - Control Plane Scheduling: The
allowSchedulingOnControlPlanessetting, previously hardcoded totruefor control plane machines, is now configurable via a new Helm value of the same name, providing greater flexibility for cluster deployments. - Default Values Update: Updated the default
values.yamlfiles for bothcozystackandgenericcharts to include the newkubespan(defaulting tofalse) andallowSchedulingOnControlPlanes(defaulting totrue) options. - Code Generation Update: The
pkg/generated/presets.gofile, which embeds Helm chart templates and values as Go strings, has been updated to reflect all the new configuration options and their default values.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces two new configuration options, kubespan and allowSchedulingOnControlPlanes, to the Helm charts and makes them configurable through values.yaml. The changes are consistently applied across cozystack and generic charts, as well as the generated Go presets. My review focuses on ensuring the new values.yaml files and their embedded counterparts in Go code adhere to common formatting standards.
charts/cozystack/values.yaml
Outdated
| kubespan: false | ||
| allowSchedulingOnControlPlanes: true |
There was a problem hiding this comment.
It's a good practice for YAML files to end with a newline character. Some tools may have issues parsing files that don't end with a newline.
kubespan: false
allowSchedulingOnControlPlanes: true
charts/generic/values.yaml
Outdated
| kubespan: false | ||
| allowSchedulingOnControlPlanes: true |
There was a problem hiding this comment.
It's a good practice for YAML files to end with a newline character. Some tools may have issues parsing files that don't end with a newline.
kubespan: false
allowSchedulingOnControlPlanes: true
pkg/generated/presets.go
Outdated
| certSANs: [] | ||
| `, | ||
| kubespan: false | ||
| allowSchedulingOnControlPlanes: true`, |
There was a problem hiding this comment.
For consistency and to adhere to YAML best practices, the embedded values.yaml content should end with a newline character (\n).
| allowSchedulingOnControlPlanes: true`, | |
| allowSchedulingOnControlPlanes: true\n`, |
pkg/generated/presets.go
Outdated
| certSANs: [] | ||
| `, | ||
| kubespan: false | ||
| allowSchedulingOnControlPlanes: true`, |
There was a problem hiding this comment.
For consistency and to adhere to YAML best practices, the embedded values.yaml content should end with a newline character (\n).
| allowSchedulingOnControlPlanes: true`, | |
| allowSchedulingOnControlPlanes: true\n`, |
![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 (2)
charts/generic/values.yaml (1)
10-10: Add missing newline at end of file.Static analysis correctly identified that the file is missing a newline character at the end.
-allowSchedulingOnControlPlanes: true +allowSchedulingOnControlPlanes: true +charts/cozystack/values.yaml (1)
14-14: Add missing newline at end of file.Same formatting issue as in the generic chart - missing newline at end of file.
-allowSchedulingOnControlPlanes: true +allowSchedulingOnControlPlanes: true +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/generated/presets.gois excluded by!**/generated/**
📒 Files selected for processing (4)
charts/cozystack/templates/_helpers.tpl(3 hunks)charts/cozystack/values.yaml(1 hunks)charts/generic/templates/_helpers.tpl(1 hunks)charts/generic/values.yaml(1 hunks)
🪛 YAMLlint (1.37.1)
charts/cozystack/values.yaml
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
charts/generic/values.yaml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/cozystack/values.yaml
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
charts/generic/values.yaml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (6)
charts/generic/values.yaml (1)
9-10: LGTM! Good configuration defaults.The new configuration keys have sensible defaults:
kubespan: falseprovides a conservative defaultallowSchedulingOnControlPlanes: truemaintains existing behaviorcharts/cozystack/values.yaml (1)
13-14: LGTM! Consistent configuration across charts.The configuration keys match those in the generic chart, ensuring consistency across the Helm charts.
charts/generic/templates/_helpers.tpl (1)
18-21: LGTM! Clean conditional template implementation.The kubespan configuration is properly conditionally included with correct Helm template syntax and appropriate indentation within the network section.
charts/cozystack/templates/_helpers.tpl (3)
58-61: LGTM! Consistent kubespan implementation.The conditional kubespan configuration matches the implementation in the generic chart, ensuring consistency across templates.
92-92: LGTM! Good refactoring from hardcoded to configurable value.Replacing the hardcoded
truewith.Values.allowSchedulingOnControlPlanesmakes this behavior configurable while maintaining the same default behavior.
115-115: LGTM! Logical connection between kubespan and discovery.Setting
discovery.enabledbased on.Values.kubespanmakes sense - when kubespan is enabled, cluster discovery should also be enabled to facilitate node discovery in the mesh network.
…iguration [feature] fix regression if default values are not supplied for kubespan (default false) or allowSchedulingOnControlPlanes (default true) Signed-off-by: lb0o <[email protected]>
…ing template rendering Signed-off-by: lb0o <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
59-59: Fix minor grammar & typo for clarityMinor editorial polish improves readability and prevents distraction.
-If you are testing template rendering you can pass the flag --offline. Also using the --full flag will generate a complete configuration, usefull for testing proper rendering of the templates. +If you are testing template rendering, you can pass the `--offline` flag. Additionally, using the `--full` flag generates a complete configuration, useful for validating the template rendering.
61-62: Place--offlinebefore>redirection for shell-style consistency
>can legally appear mid-command, but placing IO-redirection last avoids surprises (e.g., when wrapping the line) and matches common CLI examples.-talm template -t templates/controlplane.yaml --full -i > nodes/cp1.yaml --offline -talm template -t templates/worker.yaml --full -i > nodes/worker1.yaml --offline +talm template --offline --full -t templates/controlplane.yaml -i > nodes/cp1.yaml +talm template --offline --full -t templates/worker.yaml -i > nodes/worker1.yaml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🪛 LanguageTool
README.md
[grammar] ~59-~59: Ensure spelling is correct
Context: ...will generate a complete configuration, usefull for testing proper rendering of the tem...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~59-~59: Ensure spelling is correct
Context: ...will generate a complete configuration, usefull for testing proper rendering of the tem...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
…endering for development. Littly hacky addition of a render-template method in order to verify and rapidly iterate development of new talm templating functionnalities. Ass the capacity to pass any parametes the talm template commands. Signed-off-by: lb0o <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
59-64: Fix typo + tighten phrasing of offline/full rendering noteSmall grammar + spelling issues slip in here. Suggest tightening the sentence and wrapping flags in code formatting.
-If you are testing template rendering you can pass the flag --offline. Also using the --full flag will generate a complete configuration, usefull for testing proper rendering of the templates. +If you are testing template rendering, you can pass the `--offline` flag. Using the `--full` flag generates a complete configuration, useful for verifying that the templates render correctly.
65-68: Improve grammar & formatting for themake render-templateexampleThe sentence reads awkwardly and misses code-style formatting for the target.
-To develop and extend templates capabilities of talm you can use make render-template to render templates offline and check the output: +To develop and extend the template capabilities of Talm, you can use the `make render-template` target to render templates offline and inspect the output:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(1 hunks)README.md(1 hunks)
🪛 LanguageTool
README.md
[grammar] ~59-~59: Ensure spelling is correct
Context: ...will generate a complete configuration, usefull for testing proper rendering of the tem...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~59-~59: Ensure spelling is correct
Context: ...will generate a complete configuration, usefull for testing proper rendering of the tem...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
1 participant
[feature] kubespan support and allowSchedulingOnControlPlanes in default values.yaml
Follow-up to #71 — addressed comments.
Summary by CodeRabbit