[fix] Fixed openwisp_user and openwisp_group variables#81
Conversation
# Conflicts: # tasks/complete.yml # tasks/user_management.yml
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThis pull request refactors the Ansible role to support configurable OpenWISP user and group names instead of hardcoded "openwisp" values. New variables Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The PR properly addresses the issue of hardcoded openwisp user/group values by:
Files Reviewed (10 files)
Updated review: No new issues in incremental changes. The added validate parameter for sudoers file is a good security improvement. Reviewed by kimi-k2.5-0127 · 285,919 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/backport.yml:
- Line 1: This PR includes unrelated linting changes (the YAML document marker
`---` and the `# yamllint disable` directive) that should be split out; remove
or revert those lint-only edits from this PR and create a separate focused PR
containing the YAML document marker and yamllint suppression so this PR only
contains the fixes for openwisp_user/openwisp_group support, ensuring clearer
review history and backporting; mention the separate PR in the current PR
description if needed.
In `@tasks/user_management.yml`:
- Around line 15-18: The task using ansible.builtin.template to write
sudoers.d/{{ openwisp_group }} must validate the rendered file with visudo
before replacing the target; update the template task (ansible.builtin.template)
in tasks/user_management.yml to add a validate step such as validate: "visudo
-cf %s" so Ansible checks syntax on the temporary file and fails safely if
rendering/substitution is invalid, keeping the existing src, dest and mode
settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35c050e2-a1f3-4564-b405-2185f3180389
📒 Files selected for processing (10)
.github/workflows/backport.ymlREADME.mddefaults/main.ymlfiles/sudoers.d/openwispmolecule/resources/verify.ymlmolecule/vars/main.ymltasks/complete.ymltasks/user_management.ymltemplates/sudoers.d/openwisp_uservars/main.yml
💤 Files with no reviewable changes (2)
- vars/main.yml
- files/sudoers.d/openwisp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Kilo Code Review
- GitHub Check: Build debian13
- GitHub Check: Analyze (python)
- GitHub Check: Build debian11
- GitHub Check: Build ubuntu2404
- GitHub Check: Build ubuntu2204
- GitHub Check: Build debian12
🧰 Additional context used
🪛 Checkov (3.2.528)
molecule/resources/verify.yml
[low] 28-44: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
🔇 Additional comments (7)
defaults/main.yml (1)
51-52: LGTM!molecule/vars/main.yml (1)
9-10: LGTM!templates/sudoers.d/openwisp_user (1)
1-1: LGTM!tasks/user_management.yml (1)
12-12: LGTM!tasks/complete.yml (1)
4-4: LGTM!molecule/resources/verify.yml (1)
28-64: LGTM!README.md (1)
176-179: LGTM!Also applies to: 250-250
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
This PR requires careful review before merging. I made these changes hastily.
In current state, the PR should be considered unfinished.
Checklist
Problem
The role exposes the following variables:
However, several tasks still hardcoded the default
openwispuser/group, which caused partial or broken support for customized
deployments.
Examples included:
become_user: openwispvars/instead ofdefaults/As a result, overriding
openwisp_userandopenwisp_groupdid notwork consistently across the role.
Goal
The main objective of this PR is to make the role fully support
customized Linux users/groups instead of implicitly depending on the
default
openwispaccount.