fix(examples): prevent play vars from overriding inventory variables#1
fix(examples): prevent play vars from overriding inventory variables#1
Conversation
Remove `cozystack_k3s_extra_args` and `cozystack_flush_iptables` from
play `vars` in all prepare playbooks. Play vars have higher precedence
than inventory vars in Ansible, so setting them in both places caused
inventory values to be silently ignored.
Use `| default("")` and `| default(false)` in templates and conditions
to provide safe fallbacks when the variables are not set.
Add a note to README about variable customization and precedence.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Summary of ChangesHello, 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 resolves a critical issue where Ansible's variable precedence mechanism caused user-defined inventory variables for K3s extra arguments and iptables flushing to be silently ignored. By refactoring the playbooks to remove these variables from the play Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an Ansible variable precedence issue by removing user-facing variables from the play's vars section and using default filters for safe fallbacks. The accompanying README update clearly explains the change to users. I've suggested a minor improvement to the Jinja2 templating in the example playbooks to make the argument string construction more robust by avoiding a potential trailing space. Overall, the changes are solid and improve the usability of the example playbooks.
| extra_server_args: >- | ||
| {{ cozystack_k3s_server_args }} | ||
| {{ cozystack_k3s_extra_args }} | ||
| {{ cozystack_k3s_extra_args | default("") }} |
There was a problem hiding this comment.
While the default("") filter prevents an error when cozystack_k3s_extra_args is not defined, this implementation adds a trailing space to extra_server_args if cozystack_k3s_extra_args is empty or not set. This is usually harmless for command-line arguments, but it's cleaner to avoid it. You can conditionally add the space and arguments only when cozystack_k3s_extra_args has a value.
extra_server_args: "{{ cozystack_k3s_server_args }}{% if cozystack_k3s_extra_args | default('') %} {{ cozystack_k3s_extra_args }}{% endif %}"| extra_server_args: >- | ||
| {{ cozystack_k3s_server_args }} | ||
| {{ cozystack_k3s_extra_args }} | ||
| {{ cozystack_k3s_extra_args | default("") }} |
There was a problem hiding this comment.
While the default("") filter prevents an error when cozystack_k3s_extra_args is not defined, this implementation adds a trailing space to extra_server_args if cozystack_k3s_extra_args is empty or not set. This is usually harmless for command-line arguments, but it's cleaner to avoid it. You can conditionally add the space and arguments only when cozystack_k3s_extra_args has a value.
extra_server_args: "{{ cozystack_k3s_server_args }}{% if cozystack_k3s_extra_args | default('') %} {{ cozystack_k3s_extra_args }}{% endif %}"| extra_server_args: >- | ||
| {{ cozystack_k3s_server_args }} | ||
| {{ cozystack_k3s_extra_args }} | ||
| {{ cozystack_k3s_extra_args | default("") }} |
There was a problem hiding this comment.
While the default("") filter prevents an error when cozystack_k3s_extra_args is not defined, this implementation adds a trailing space to extra_server_args if cozystack_k3s_extra_args is empty or not set. This is usually harmless for command-line arguments, but it's cleaner to avoid it. You can conditionally add the space and arguments only when cozystack_k3s_extra_args has a value.
extra_server_args: "{{ cozystack_k3s_server_args }}{% if cozystack_k3s_extra_args | default('') %} {{ cozystack_k3s_extra_args }}{% endif %}"
📝 WalkthroughWalkthroughDocumentation expanded with variable scope guidance, and three Ansible playbooks refactored to remove hardcoded variable declarations, replacing them with safe Jinja2 default filters to handle undefined inventory variables gracefully. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/rhel/prepare-rhel.yml (1)
84-97: Hardenwhenguards with explicit boolean coercion.All three iptables task guards at lines 84, 91, and 97 lack the
| boolfilter. Whencozystack_flush_iptablesis passed as a string from CLI or environment (e.g.,-e cozystack_flush_iptables=false), the string"false"evaluates as truthy, causing tasks to run unexpectedly. Append| boolto each guard:when: cozystack_flush_iptables | default(false) | bool🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/rhel/prepare-rhel.yml` around lines 84 - 97, The when guards for the iptables tasks use cozystack_flush_iptables | default(false) which can treat string values like "false" as truthy; for each iptables-related task (e.g., the task named "Flush iptables INPUT chain", the subsequent iptables flush task, and "Set iptables INPUT policy to ACCEPT") update their when expressions to cozystack_flush_iptables | default(false) | bool so the variable is explicitly coerced to a boolean and string "false"/"true" values behave correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/rhel/prepare-rhel.yml`:
- Around line 84-97: The when guards for the iptables tasks use
cozystack_flush_iptables | default(false) which can treat string values like
"false" as truthy; for each iptables-related task (e.g., the task named "Flush
iptables INPUT chain", the subsequent iptables flush task, and "Set iptables
INPUT policy to ACCEPT") update their when expressions to
cozystack_flush_iptables | default(false) | bool so the variable is explicitly
coerced to a boolean and string "false"/"true" values behave correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7260e41f-0192-4505-85b2-33e3ccc1a4c7
📒 Files selected for processing (4)
README.mdexamples/rhel/prepare-rhel.ymlexamples/suse/prepare-suse.ymlexamples/ubuntu/prepare-ubuntu.yml
1 participant
Summary
cozystack_k3s_extra_argsandcozystack_flush_iptablesfrom playvarsin all three prepare playbooks (ubuntu, rhel, suse)| default("")and| default(false)for safe fallbacks when variables are not setProblem
Ansible play
varshave higher precedence than inventory variables. Whencozystack_k3s_extra_args: ""was defined in the playvarssection, any value set in the inventory (e.g.,--tls-san=<public_ip>) was silently ignored. This caused k3s to generate TLS certificates without the user-specified SANs.Test plan
cozystack_k3s_extra_argsfrom inventory is applied to k3s service argscozystack_flush_iptables: truefrom inventory triggers iptables flushSummary by CodeRabbit
Release Notes
Documentation
Improvements