[bug/qa] Fix lint errors being ignored (continuation of #571) #600
Conversation
WalkthroughThis PR centralizes QA checks and standardizes YAML/Ansible formatting across the role. It adds Flake8 configuration (.flake8, pyproject.toml), expands yamllint rules, updates .ansible-lint quoting, removes lint blocks from molecule configs, and adds/adjusts a run-qa-checks script and CI step changes to run QA centrally. Many Ansible tasks and templates were normalized (file mode strings, quoting, community.general.django_manage usage), defaults/main.yml was restructured from flat lists to nested mappings, documentation for running QA was added, and minor formatting/dependency adjustments were made across CI, molecule, and resource files. Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as "GitHub Actions (CI)"
participant Script as "run-qa-checks"
participant Linters as "linters (flake8, yamllint, ansible-lint)"
participant Tests as "Test runner / Molecule"
Dev->>GH: Push / PR
GH->>GH: Install python deps (id: deps)
GH->>Script: Invoke run-qa-checks
Script->>Linters: Run linters (flake8, yamllint, ansible-lint)
Linters-->>Script: Lint results (pass/fail)
Script-->>GH: Return QA status
GH->>Tests: Run tests (only if deps succeeded and not cancelled)
Tests-->>GH: Test results
GH-->>Dev: Report combined CI status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (4)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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 |
There was a problem hiding this comment.
Pull request overview
This PR continues and completes the work started in #571 to ensure QA and lint checks are not ignored and CI failures are real and actionable. It addresses issue #485 by removing suppressed QA checks and fixing all resulting lint errors.
Changes:
- Removed all QA check skips from the
run-qa-checksscript, enabling full enforcement of code quality standards - Fixed YAML formatting issues across all Ansible files including line endings (CRLF to LF), trailing spaces, document start markers, and octal mode value quoting
- Added comprehensive flake8 and yamllint configuration files with appropriate exclusions for templates and virtual environments
- Updated CI workflow to properly fail on QA errors by removing
continue-on-errorflag and adding conditional test execution - Moved block-level
whenandtagsclauses before theblockkeyword for improved readability and consistency - Updated one instance of
django_manageto use FQCNcommunity.general.django_managein handlers
Reviewed changes
Copilot reviewed 27 out of 33 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| run-qa-checks | Removed all QA skip flags to enforce lint checks |
| pyproject.toml | Added flake8 configuration section |
| .flake8 | Added comprehensive flake8 exclusion configuration |
| .yamllint.yml | Expanded yamllint rules and ignore patterns |
| .ansible-lint | Fixed YAML formatting |
| .github/workflows/ci.yml | Removed continue-on-error and added conditional execution |
| .github/dependabot.yml | Fixed YAML document start marker |
| .github/FUNDING.yml | Fixed YAML document start marker |
| tasks/*.yml | Fixed YAML formatting, removed blank lines, quoted mode values |
| handlers/main.yml | Updated django_manage to use FQCN, fixed YAML formatting |
| molecule/*/molecule.yml | Removed inline lint blocks, fixed YAML formatting |
| molecule/resources/*.yml | Fixed YAML formatting |
| defaults/main.yml | Fixed indentation and changed null to empty values |
| meta/main.yml | Fixed YAML document start marker |
| vars/debian-13.yml | Removed trailing spaces, fixed document start marker |
| templates/openwisp2/settings.py | Line number updates only (template excluded from linting) |
| docs/developer/installation.rst | Added QA checks documentation section |
| README.md | Line number updates only |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
run-qa-checks
Outdated
| --skip-black \ | ||
| --skip-checkmigrations | ||
| openwisp-qa-check | ||
|
|
There was a problem hiding this comment.
There is a trailing whitespace on line 5. This should be removed to maintain consistency with the QA standards being enforced by this PR.
pyproject.toml
Outdated
| ] | ||
|
No newline at end of file |
There was a problem hiding this comment.
There is a trailing whitespace on line 11. This should be removed to maintain consistency with the QA standards being enforced by this PR.
| ] | |
| ] |
tasks/django.yml
Outdated
| django_manage: | ||
| app_path: "{{ openwisp2_path }}" | ||
| command: "check --deploy --tag geocoding" | ||
| virtualenv: "{{ virtualenv_path }}" | ||
| - name: Migrate | ||
| notify: Reload application | ||
| become: true | ||
| become_user: "{{ www_user }}" | ||
| django_manage: | ||
| app_path: "{{ openwisp2_path }}" | ||
| command: migrate | ||
| virtualenv: "{{ virtualenv_path }}" | ||
| tags: [django_settings] | ||
| - name: Set permissions to sqlite db | ||
| when: openwisp2_database.engine == "django.db.backends.sqlite3" | ||
| file: | ||
| path: "{{ openwisp2_database.name }}" | ||
| state: file | ||
| group: "{{ www_group }}" | ||
| mode: 0770 | ||
| - name: Set permissions to "{{ openwisp2_path }}/static" | ||
| file: | ||
| path: "{{ openwisp2_path }}/static" | ||
| state: directory | ||
| group: "{{ www_group }}" | ||
| owner: "{{ www_user }}" | ||
| # sets executable only to directories | ||
| mode: u=rwX,g=rX,o=rX | ||
| recurse: true | ||
| tags: | ||
| - molecule-idempotence-notest | ||
| - openwisp2_theme | ||
| - name: Check if ow2_static exists | ||
| stat: | ||
| path: "{{ role_path }}/files/ow2_static" | ||
| register: ow2_static_dir | ||
| tags: | ||
| - openwisp2_theme | ||
| - name: Copy static files | ||
| become: true | ||
| copy: | ||
| src: ow2_static/ | ||
| dest: "{{ openwisp2_path }}/static_custom" | ||
| owner: "{{ www_user }}" | ||
| group: "{{ www_group }}" | ||
| mode: 0640 | ||
| when: ow2_static_dir.stat.exists | ||
| tags: | ||
| - openwisp2_theme | ||
| - name: Collectstatic | ||
| notify: Reload application | ||
| become: true | ||
| become_user: "{{ www_user }}" | ||
| django_manage: | ||
| app_path: "{{ openwisp2_path }}" | ||
| command: "collectstatic --noinput" | ||
| virtualenv: "{{ virtualenv_path }}" | ||
| register: collectstatic_output | ||
| changed_when: '"\n0 static files" not in collectstatic_output.out' | ||
| tags: | ||
| - molecule-idempotence-notest | ||
| - openwisp2_theme | ||
| # needed to run compilemessages | ||
| # some .mo files are created in virtualenv_path | ||
| - name: Set permissions to "{{ virtualenv_path }}" | ||
| when: openwisp2_internationalization | ||
| file: | ||
| path: "{{ virtualenv_path }}" | ||
| state: directory | ||
| group: "{{ www_group }}" | ||
| owner: "{{ www_user }}" | ||
| # sets executable only to directories | ||
| mode: u=rwX,g=rX,o=rX | ||
| recurse: true | ||
| tags: | ||
| - molecule-idempotence-notest | ||
| - name: Compilemessages | ||
| notify: Reload application | ||
| become: true | ||
| become_user: "{{ www_user }}" | ||
| when: openwisp2_internationalization | ||
| django_manage: | ||
| app_path: "{{ openwisp2_path }}" | ||
| command: "compilemessages" | ||
| virtualenv: "{{ virtualenv_path }}" |
There was a problem hiding this comment.
The django_manage module usage in handlers/main.yml has been updated to use the FQCN (community.general.django_manage), but the usage in tasks/django.yml still uses the short form. For consistency and to avoid potential deprecation warnings in future Ansible versions, all django_manage module usages should use the fully qualified collection name.
defaults/main.yml
Outdated
| openwisp2_celery_autoscale: | ||
| openwisp2_celery_prefetch_multiplier: | ||
| openwisp2_celery_optimization: default | ||
| openwisp2_celery_network: true | ||
| openwisp2_celery_network_concurrency: 1 | ||
| openwisp2_celery_network_autoscale: | ||
| openwisp2_celery_network_prefetch_multiplier: 10 | ||
| openwisp2_celery_network_optimization: fair | ||
| openwisp2_celery_firmware_upgrader: true | ||
| openwisp2_celery_firmware_upgrader_concurrency: 1 | ||
| openwisp2_celery_firmware_upgrader_autoscale: | ||
| openwisp2_celery_firmware_upgrader_prefetch_multiplier: 1 | ||
| openwisp2_celery_firmware_upgrader_optimization: fair | ||
| openwisp2_celery_monitoring: true | ||
| openwisp2_celery_monitoring_concurrency: 2 | ||
| openwisp2_celery_monitoring_autoscale: | ||
| openwisp2_celery_monitoring_prefetch_multiplier: 10 | ||
| openwisp2_celery_monitoring_optimization: fair | ||
| openwisp2_celery_task_routes_defaults: true | ||
| openwisp2_celery_broker_url: redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/3 | ||
| openwisp2_celery_task_acks_late: true | ||
| openwisp2_celery_broker_max_tries: 10 | ||
| openwisp2_celery_pool: | ||
| openwisp2_celery_monitoring_pool: | ||
| openwisp2_celery_network_pool: | ||
| openwisp2_celery_firmware_upgrader_pool: | ||
| openwisp2_django_celery_logging: false | ||
| openwisp2_postfix_install: true | ||
| postfix_smtpd_relay_restrictions_override: "permit_sasl_authenticated, permit_mynetworks, check_relay_domains, reject_unauth_destination, reject" | ||
| openwisp2_uwsgi_gid: | ||
| openwisp2_uwsgi_processes: 1 | ||
| openwisp2_uwsgi_threads: 1 | ||
| openwisp2_uwsgi_listen: 100 | ||
| openwisp2_uwsgi_socket: "{{ openwisp2_path }}/uwsgi.sock" | ||
| openwisp2_uwsgi_extra_conf: | | ||
| single-interpreter=True | ||
| log-4xx=True | ||
| log-5xx=True | ||
| disable-logging=True | ||
| auto-procname=True | ||
| openwisp2_daphne_install: true | ||
| openwisp2_daphne_socket: "unix://{{ openwisp2_path }}/daphne0.sock" | ||
| openwisp2_daphne_processes: 1 | ||
| openwisp2_daphne_websocket_timeout: 3600 # in seconds | ||
| # nginx configuration | ||
| openwisp2_http_allowed_ip: false | ||
| openwisp2_admin_allowed_network: |
There was a problem hiding this comment.
Variables with empty values (lines 86, 87, 91, 96, 101, 108-111, 115, 132) may cause issues. In YAML, a key with no value after the colon is interpreted as null/None, which is semantically the same as explicitly setting 'null'. However, for clarity and consistency with the rest of the codebase, these should explicitly use 'null' or omit the keys entirely if they are meant to be optional. The yamllint configuration forbids empty values in block mappings, though these are at the root level. Consider being explicit about null values for clarity.
tasks/django.yml
Outdated
| mode: 0770 | ||
| - name: Create "{{ openwisp2_path }}/openwisp2" | ||
| file: | ||
| path: "{{ openwisp2_path }}/openwisp2" | ||
| state: directory | ||
| group: "{{ www_group }}" | ||
| mode: 0770 | ||
| - name: Create "{{ openwisp2_path }}/log" | ||
| file: | ||
| path: "{{ openwisp2_path }}/log" | ||
| state: directory | ||
| group: "{{ www_group }}" | ||
| mode: 0770 | ||
| recurse: true | ||
| tags: | ||
| - molecule-idempotence-notest | ||
| - name: Create "{{ openwisp2_path }}/static_custom" | ||
| file: | ||
| path: "{{ openwisp2_path }}/static_custom" | ||
| state: directory | ||
| group: "{{ www_group }}" | ||
| mode: 0770 | ||
| tags: | ||
| - openwisp2_theme | ||
| - name: Template manage.py | ||
| notify: Reload application | ||
| template: | ||
| src: manage.py | ||
| dest: "{{ openwisp2_path }}/manage.py" | ||
| group: "{{ www_group }}" | ||
| mode: 0754 | ||
| - name: Template celery.py | ||
| notify: Reload application | ||
| template: | ||
| src: openwisp2/celery.py | ||
| dest: "{{ openwisp2_path }}/openwisp2/celery.py" | ||
| group: "{{ www_group }}" | ||
| mode: 0640 | ||
| tags: [django_init] | ||
| - name: Template version.py | ||
| notify: Reload application | ||
| template: | ||
| src: openwisp2/version.py | ||
| dest: "{{ openwisp2_path }}/openwisp2/version.py" | ||
| group: "{{ www_group }}" | ||
| mode: 0640 | ||
| tags: [django_init] | ||
| - name: Template __init__.py | ||
| notify: Reload application | ||
| template: | ||
| src: openwisp2/__init__.py | ||
| dest: "{{ openwisp2_path }}/openwisp2/__init__.py" | ||
| group: "{{ www_group }}" | ||
| mode: 0640 | ||
| tags: [django_init] | ||
| - name: Template urls.py | ||
| notify: Reload application | ||
| template: | ||
| src: openwisp2/urls.py | ||
| dest: "{{ openwisp2_path }}/openwisp2/urls.py" | ||
| group: "{{ www_group }}" | ||
| mode: 0640 | ||
| - name: Template wsgi.py | ||
| notify: Reload application | ||
| template: | ||
| src: openwisp2/wsgi.py | ||
| dest: "{{ openwisp2_path }}/openwisp2/wsgi.py" | ||
| group: "{{ www_group }}" | ||
| mode: 0640 | ||
| - name: Template asgi.py | ||
| notify: Reload application | ||
| template: | ||
| src: openwisp2/asgi.py | ||
| dest: "{{ openwisp2_path }}/openwisp2/asgi.py" | ||
| group: "{{ www_group }}" | ||
| mode: 0640 | ||
| # set openwisp2_secret_key if not defined explicitly | ||
| - import_tasks: django_secret_key.yml | ||
| when: openwisp2_secret_key is not defined | ||
| tags: [django_settings] | ||
| - name: Template settings.py | ||
| notify: Reload application | ||
| template: | ||
| src: openwisp2/settings.py | ||
| dest: "{{ openwisp2_path }}/openwisp2/settings.py" | ||
| group: "{{ www_group }}" | ||
| mode: 0640 | ||
| tags: [django_settings] | ||
| - name: Template routing.py | ||
| notify: Reload application | ||
| template: | ||
| src: openwisp2/routing.py | ||
| dest: "{{ openwisp2_path }}/openwisp2/routing.py" | ||
| group: "{{ www_group }}" | ||
| mode: 0640 | ||
| - name: Start redis for migration | ||
| when: openwisp2_redis_install | ||
| service: | ||
| name: redis | ||
| state: started | ||
| - name: Run geocoding check | ||
| become: true | ||
| become_user: "{{ www_user }}" | ||
| when: openwisp2_geocoding_check | ||
| django_manage: | ||
| app_path: "{{ openwisp2_path }}" | ||
| command: "check --deploy --tag geocoding" | ||
| virtualenv: "{{ virtualenv_path }}" | ||
| - name: Migrate | ||
| notify: Reload application | ||
| become: true | ||
| become_user: "{{ www_user }}" | ||
| django_manage: | ||
| app_path: "{{ openwisp2_path }}" | ||
| command: migrate | ||
| virtualenv: "{{ virtualenv_path }}" | ||
| tags: [django_settings] | ||
| - name: Set permissions to sqlite db | ||
| when: openwisp2_database.engine == "django.db.backends.sqlite3" | ||
| file: | ||
| path: "{{ openwisp2_database.name }}" | ||
| state: file | ||
| group: "{{ www_group }}" | ||
| mode: 0770 | ||
| - name: Set permissions to "{{ openwisp2_path }}/static" | ||
| file: | ||
| path: "{{ openwisp2_path }}/static" | ||
| state: directory | ||
| group: "{{ www_group }}" | ||
| owner: "{{ www_user }}" | ||
| # sets executable only to directories | ||
| mode: u=rwX,g=rX,o=rX | ||
| recurse: true | ||
| tags: | ||
| - molecule-idempotence-notest | ||
| - openwisp2_theme | ||
| - name: Check if ow2_static exists | ||
| stat: | ||
| path: "{{ role_path }}/files/ow2_static" | ||
| register: ow2_static_dir | ||
| tags: | ||
| - openwisp2_theme | ||
| - name: Copy static files | ||
| become: true | ||
| copy: | ||
| src: ow2_static/ | ||
| dest: "{{ openwisp2_path }}/static_custom" | ||
| owner: "{{ www_user }}" | ||
| group: "{{ www_group }}" | ||
| mode: 0640 | ||
| when: ow2_static_dir.stat.exists | ||
| tags: | ||
| - openwisp2_theme | ||
| - name: Collectstatic | ||
| notify: Reload application | ||
| become: true | ||
| become_user: "{{ www_user }}" | ||
| django_manage: | ||
| app_path: "{{ openwisp2_path }}" | ||
| command: "collectstatic --noinput" | ||
| virtualenv: "{{ virtualenv_path }}" | ||
| register: collectstatic_output | ||
| changed_when: '"\n0 static files" not in collectstatic_output.out' | ||
| tags: | ||
| - molecule-idempotence-notest | ||
| - openwisp2_theme | ||
| # needed to run compilemessages | ||
| # some .mo files are created in virtualenv_path | ||
| - name: Set permissions to "{{ virtualenv_path }}" | ||
| when: openwisp2_internationalization | ||
| file: | ||
| path: "{{ virtualenv_path }}" | ||
| state: directory | ||
| group: "{{ www_group }}" | ||
| owner: "{{ www_user }}" | ||
| # sets executable only to directories | ||
| mode: u=rwX,g=rX,o=rX | ||
| recurse: true | ||
| tags: | ||
| - molecule-idempotence-notest | ||
| - name: Compilemessages | ||
| notify: Reload application | ||
| become: true | ||
| become_user: "{{ www_user }}" | ||
| when: openwisp2_internationalization | ||
| django_manage: | ||
| app_path: "{{ openwisp2_path }}" | ||
| command: "compilemessages" | ||
| virtualenv: "{{ virtualenv_path }}" | ||
| - name: Create load_initial_data.py script | ||
| template: | ||
| src: load_initial_data.py | ||
| dest: "{{ openwisp2_path }}/load_initial_data.py" | ||
| mode: 0754 |
There was a problem hiding this comment.
The yamllint configuration at line 24-25 sets forbid-implicit-octal: true and forbid-explicit-octal: true, which would forbid octal mode values like mode: 0770 used throughout this file. However, these mode values remain unquoted. Either the yamllint configuration should be adjusted to allow octal values with proper quoting (by setting forbid-explicit-octal to false), or all mode values in this file should be quoted as strings (e.g., "0770"). Based on the changes in other task files where mode values are quoted, it appears the intention is to quote all mode values, but this file was missed.
pyproject.toml
Outdated
| extend_exclude = ["**/*.py"] | ||
|
|
||
| [tool.flake8] | ||
| exclude = [ | ||
| ".venv", | ||
| "venv", | ||
| "__pycache__", | ||
| ".git", | ||
| ] | ||
|
No newline at end of file |
There was a problem hiding this comment.
There are two flake8 configuration files: pyproject.toml with a [tool.flake8] section and .flake8. Flake8 does not natively support pyproject.toml configuration (it only reads from .flake8, setup.cfg, or tox.ini). The [tool.flake8] section in pyproject.toml will be ignored by flake8 unless a plugin like flake8-pyproject is installed. Consider removing the duplicate configuration from pyproject.toml or documenting the requirement for the flake8-pyproject plugin in requirements-test.txt.
| extend_exclude = ["**/*.py"] | |
| [tool.flake8] | |
| exclude = [ | |
| ".venv", | |
| "venv", | |
| "__pycache__", | |
| ".git", | |
| ] | |
| extend_exclude = ["**/*.py"] |
templates/openwisp2/settings.py
Outdated
|
|
||
| TEST_RUNNER = "openwisp_utils.metric_collection.tests.runner.MockRequestPostRunner" | ||
| SECURE_REFERRER_POLICY = "strict-origin" | ||
| import os |
There was a problem hiding this comment.
Syntax Error (in Python 3).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In @.github/dependabot.yml:
- Line 13: The configuration's interval value doesn't match its inline comment:
update the interval setting named "interval" so it matches the intended cadence
in the comment (change "monthly" to "weekly") or adjust the comment to reflect
the actual value; specifically edit the "interval" entry in the dependabot
configuration so the value and the comment are consistent.
- Line 7: The inline comment is inconsistent with the dependabot setting: the
key interval: "monthly" does not match the comment "Check for updates weekly";
update the comment to reflect the actual configuration (change the comment to
"Check for updates monthly") or, if weekly updates are intended, change the
interval value to "weekly" so the comment and the interval: "monthly" setting
for dependabot are consistent; edit the line containing interval: "monthly" and
its adjacent comment accordingly.
In @.github/workflows/ci.yml:
- Around line 38-41: The step name uses an undefined matrix variable
"matrix.python-version" which should be corrected to match the defined matrix
value; update the step name "Set up Python ${{ matrix.python-version }}" to
reference the actual matrix key (e.g., "Set up Python ${{ matrix.distro }}" or
simply "Set up Python") or alternatively add a "python-version" entry to the
matrix so the interpolation matches; locate the workflow step with the name
string and make the name and matrix keys consistent.
In `@handlers/main.yml`:
- Around line 48-55: Add a machine-readable collection requirement so the play
using community.general.django_manage always has the collection installed:
update the role metadata by adding a galaxy_info.collections entry that declares
community.general with version ">=3.6.0" (and also add ansible.posix) under the
role's meta configuration so the dependency is enforced for handlers that call
the django_manage module; ensure the collections list includes the names and
version constraint exactly as specified.
In `@README.md`:
- Around line 3-5: Replace the insecure HTTP Galaxy badge image URL with HTTPS:
find the markdown line starting with the "[![Galaxy]" badge (the image URL
currently
"http://img.shields.io/badge/galaxy-openwisp.openwisp2-blue.svg?style=flat-square")
and update it to use "https://", ensuring the badge image is loaded securely
like the other badges.
In `@run-qa-checks`:
- Line 5: Remove the trailing whitespace at the end of the file "run-qa-checks"
(the extra space at EOF) so the file ends cleanly with no spaces or blank
characters after the last line; simply delete the trailing space and save the
file.
In `@tasks/cron.yml`:
- Around line 5-7: Update the typo in the cron job name: change the value of the
cron.name string "Update toplogies" to "Update topologies" so the cron entry is
correctly spelled; edit the cron mapping where the name key is defined (cron ->
name) to use the corrected spelling.
In `@tasks/freeradius.yml`:
- Around line 52-61: The task "Add freeradius repository into Debian bookworm
sources list" uses an unnecessary sudo inside the shell command while already
declaring become: true; remove the "sudo" and the backslash continuation so the
command simply echoes to /etc/apt/sources.list.d/networkradius.list under
privilege escalation. Ensure the shell entry matches the bullseye task style (no
sudo) and keep become: true so the file is created with elevated privileges.
- Around line 74-83: The task "Add Network RADIUS PGP public key" currently uses
ignore_errors: true while registering result and using until: result is success,
which masks failures; remove ignore_errors: true so the playbook will fail if
the get_url task (register: result, until: result is success, retries/delay)
cannot download the key, or alternatively keep ignore_errors but add an explicit
verification step after the task (e.g., an assert or stat checking the presence
of /etc/apt/trusted.gpg.d/packages.networkradius.com.asc and failing the
playbook if not found) before any apt update/install tasks that depend on the
signed-by key.
- Around line 195-201: Replace the non-idiomatic include_tasks path
"tasks/freeradius_eap.yml" with the filename only "freeradius_eap.yml" in the
task that loops over freeradius_eap_orgs (the task with include_tasks, loop: "{{
freeradius_eap_orgs }}", loop_control.loop_var: org and tags: [freeradius_eap])
so the role includes the task file from the role’s tasks/ directory in the
standard Ansible way; keep the loop, loop_control and when condition unchanged.
In `@tasks/nginx.yml`:
- Around line 26-36: The task "Create SSL cert if not exists yet" embeds
creates={{ openwisp2_ssl_cert }} inside the openssl command string, so Ansible
won't treat it as the command module's creates argument; remove the trailing
"creates=..." from the command value and add a separate top-level creates: "{{
openwisp2_ssl_cert }}" key under the same task (keeping the command string
intact and the notify: Restart nginx), so the command module will skip execution
when the certificate file exists.
🧹 Nitpick comments (1)
tasks/django.yml (1)
8-8: Consider quoting octal mode values for consistency.Ansible recommends quoting file mode values as strings (e.g.,
mode: "0770") to avoid potential YAML parser ambiguities. This pattern appears throughout the file on lines 8, 15, 22, 32, 42, 50, 59, 68, 77, 85, 93, 106, 115, 148, 157, 177, and 225.Note: The
tasks/django_secret_key.ymlfile in this PR already uses quoted modes (mode: "0754"), so aligning this file would improve consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.ansible-lint.flake8.github/FUNDING.yml.github/dependabot.yml.github/workflows/ci.yml.yamllint.ymlREADME.mddefaults/main.ymldocs/developer/installation.rsthandlers/main.ymlmeta/main.ymlmolecule/default/molecule.ymlmolecule/local/molecule.ymlmolecule/resources/converge.ymlmolecule/resources/requirements.ymlmolecule/resources/verify.ymlpyproject.tomlrun-qa-checkstasks/apt.ymltasks/complete.ymltasks/cron.ymltasks/django.ymltasks/django_secret_key.ymltasks/freeradius.ymltasks/freeradius_eap.ymltasks/main.ymltasks/nginx.ymltasks/pip.ymltasks/ssh.ymltasks/supervisor.ymltasks/system.ymltemplates/openwisp2/settings.pyvars/debian-13.yml
🧰 Additional context used
🪛 actionlint (1.7.10)
.github/workflows/ci.yml
[error] 38-38: property "python-version" is not defined in object type {distro: string}
(expression)
🪛 Checkov (3.2.334)
molecule/resources/verify.yml
[medium] 50-55: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
🪛 Ruff (0.14.14)
templates/openwisp2/settings.py
[warning] 20-20: Expected an expression
(invalid-syntax)
[warning] 20-20: Expected an identifier, but found a keyword for that cannot be used here
(invalid-syntax)
[warning] 20-20: Expected ,, found name
(invalid-syntax)
[warning] 20-20: Expected an expression
(invalid-syntax)
[warning] 21-21: Expected ,, found string
(invalid-syntax)
[warning] 22-22: Expected an expression
(invalid-syntax)
[warning] 22-22: Expected an expression
(invalid-syntax)
[warning] 50-50: Expected an expression
(invalid-syntax)
[warning] 50-50: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 50-50: Expected ,, found name
(invalid-syntax)
[warning] 50-50: Expected an expression
(invalid-syntax)
[warning] 51-51: Expected ,, found string
(invalid-syntax)
[warning] 52-52: Expected an expression
(invalid-syntax)
[warning] 52-52: Expected an expression
(invalid-syntax)
[warning] 53-53: Expected ,, found {
(invalid-syntax)
[warning] 53-53: Expected an expression
(invalid-syntax)
[warning] 53-53: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 53-53: Expected ,, found name
(invalid-syntax)
[warning] 53-53: Expected an expression
(invalid-syntax)
[warning] 54-54: Expected ,, found string
(invalid-syntax)
[warning] 58-58: Expected an expression
(invalid-syntax)
[warning] 58-58: Expected an expression
(invalid-syntax)
[warning] 59-59: Expected ,, found string
(invalid-syntax)
[warning] 61-61: Expected an expression
(invalid-syntax)
[warning] 61-61: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 61-61: Expected ,, found name
(invalid-syntax)
[warning] 61-61: Expected an expression
(invalid-syntax)
[warning] 62-62: Expected ,, found string
(invalid-syntax)
[warning] 63-63: Expected an expression
(invalid-syntax)
[warning] 63-63: Expected an expression
(invalid-syntax)
[warning] 64-64: Expected ,, found {
(invalid-syntax)
[warning] 64-64: Expected an expression
(invalid-syntax)
[warning] 64-64: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 64-64: Expected ,, found name
(invalid-syntax)
[warning] 64-64: Expected an expression
(invalid-syntax)
[warning] 65-65: Expected ,, found string
(invalid-syntax)
[warning] 66-66: Expected an expression
(invalid-syntax)
[warning] 66-66: Expected an expression
(invalid-syntax)
[warning] 67-67: Expected ,, found string
(invalid-syntax)
[warning] 68-68: Expected an expression
(invalid-syntax)
[warning] 68-68: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 68-68: Expected ,, found name
(invalid-syntax)
[warning] 68-68: Expected an expression
(invalid-syntax)
[warning] 69-69: Expected ,, found string
(invalid-syntax)
[warning] 72-72: Expected an expression
(invalid-syntax)
[warning] 72-72: Expected an expression
(invalid-syntax)
[warning] 75-75: Expected ,, found string
(invalid-syntax)
[warning] 76-76: Expected an expression
(invalid-syntax)
[warning] 76-76: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 76-76: Expected ,, found name
(invalid-syntax)
[warning] 76-76: Expected an expression
(invalid-syntax)
[warning] 77-77: Expected ,, found string
(invalid-syntax)
[warning] 78-78: Expected an expression
(invalid-syntax)
[warning] 78-78: Expected an expression
(invalid-syntax)
[warning] 79-79: Expected ,, found string
(invalid-syntax)
[warning] 91-91: Expected an expression
(invalid-syntax)
[warning] 91-91: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 91-91: Expected ,, found name
(invalid-syntax)
[warning] 91-91: Expected an expression
(invalid-syntax)
[warning] 92-92: Expected ,, found string
(invalid-syntax)
[warning] 93-93: Expected an expression
(invalid-syntax)
[warning] 93-93: Expected an expression
(invalid-syntax)
[warning] 94-94: Expected ,, found string
(invalid-syntax)
[warning] 98-98: Expected an expression
(invalid-syntax)
[warning] 98-98: Expected an identifier, but found a keyword for that cannot be used here
(invalid-syntax)
[warning] 98-98: Expected ,, found name
(invalid-syntax)
[warning] 98-98: Expected an expression
(invalid-syntax)
[warning] 99-99: Expected ,, found string
(invalid-syntax)
[warning] 100-100: Expected an expression
(invalid-syntax)
[warning] 100-100: Expected an expression
(invalid-syntax)
[warning] 101-101: Expected ,, found {
(invalid-syntax)
[warning] 101-101: Expected an expression
(invalid-syntax)
[warning] 101-101: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 101-101: Expected ,, found name
(invalid-syntax)
[warning] 101-101: Expected an expression
(invalid-syntax)
[warning] 102-102: Expected ,, found string
(invalid-syntax)
[warning] 103-103: Expected an expression
(invalid-syntax)
[warning] 103-103: Expected an expression
(invalid-syntax)
[warning] 104-104: Expected ,, found {
(invalid-syntax)
[warning] 104-104: Expected an expression
(invalid-syntax)
[warning] 104-104: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 104-104: Expected ,, found name
(invalid-syntax)
[warning] 104-104: Expected an expression
(invalid-syntax)
[warning] 105-105: Expected ,, found string
(invalid-syntax)
[warning] 106-106: Expected an expression
(invalid-syntax)
[warning] 106-106: Expected an expression
(invalid-syntax)
[warning] 114-114: Expected an expression
(invalid-syntax)
[warning] 114-114: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 114-114: Expected ,, found name
(invalid-syntax)
[warning] 114-114: Expected an expression
(invalid-syntax)
[warning] 116-116: Expected an expression
(invalid-syntax)
[warning] 116-116: Expected an expression
(invalid-syntax)
[warning] 118-118: Expected an expression
(invalid-syntax)
[warning] 118-118: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 118-118: Expected ,, found name
(invalid-syntax)
[warning] 118-118: Expected an expression
(invalid-syntax)
[warning] 120-120: Expected an expression
(invalid-syntax)
[warning] 120-120: Expected an expression
(invalid-syntax)
[warning] 138-138: Expected an expression
(invalid-syntax)
[warning] 138-138: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 138-138: Expected ,, found name
(invalid-syntax)
[warning] 138-138: Expected an expression
(invalid-syntax)
[warning] 139-139: Expected ,, found string
(invalid-syntax)
[warning] 140-140: Expected an expression
(invalid-syntax)
[warning] 140-140: Expected an expression
(invalid-syntax)
[warning] 141-141: Expected ,, found string
(invalid-syntax)
[warning] 144-144: Expected an expression
(invalid-syntax)
[warning] 144-144: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 144-144: Expected ,, found name
(invalid-syntax)
[warning] 144-144: Expected an expression
(invalid-syntax)
[warning] 145-145: Expected ,, found string
(invalid-syntax)
[warning] 146-146: Expected an expression
(invalid-syntax)
[warning] 146-146: Expected an expression
(invalid-syntax)
[warning] 147-147: Expected ,, found string
(invalid-syntax)
[warning] 150-150: Expected an expression
(invalid-syntax)
[warning] 150-150: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 150-150: Expected ,, found name
(invalid-syntax)
[warning] 150-150: Expected an expression
(invalid-syntax)
[warning] 151-151: Expected ,, found string
(invalid-syntax)
[warning] 152-152: Expected an expression
(invalid-syntax)
[warning] 152-152: Expected an expression
(invalid-syntax)
[warning] 153-153: Expected ,, found string
(invalid-syntax)
[warning] 160-160: Expected an expression
(invalid-syntax)
[warning] 160-160: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 160-160: Expected ,, found name
(invalid-syntax)
[warning] 160-160: Expected an expression
(invalid-syntax)
[warning] 170-170: Expected an expression
(invalid-syntax)
[warning] 170-170: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 170-170: Expected ,, found name
(invalid-syntax)
[warning] 170-170: Expected an expression
(invalid-syntax)
[warning] 172-172: Expected an expression
(invalid-syntax)
[warning] 172-172: Expected an expression
(invalid-syntax)
[warning] 173-173: Expected an expression
(invalid-syntax)
[warning] 173-173: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 173-173: Expected ,, found name
(invalid-syntax)
[warning] 173-173: Expected an expression
(invalid-syntax)
[warning] 175-175: Expected an expression
(invalid-syntax)
[warning] 175-175: Expected an expression
(invalid-syntax)
[warning] 184-184: Expected an expression
(invalid-syntax)
[warning] 184-184: Expected an expression
(invalid-syntax)
[warning] 188-188: Expected an expression
(invalid-syntax)
[warning] 188-188: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 188-188: Expected ,, found name
(invalid-syntax)
[warning] 188-188: Expected an expression
(invalid-syntax)
[warning] 190-190: Expected an expression
(invalid-syntax)
[warning] 190-190: Expected an expression
(invalid-syntax)
[warning] 191-191: Expected an expression
(invalid-syntax)
[warning] 191-191: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 191-191: Expected ,, found name
(invalid-syntax)
[warning] 191-191: Expected an expression
(invalid-syntax)
[warning] 193-193: Expected an expression
(invalid-syntax)
[warning] 193-193: Expected an expression
(invalid-syntax)
[warning] 250-250: Expected an expression
(invalid-syntax)
[warning] 250-250: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 250-250: Expected ,, found name
(invalid-syntax)
[warning] 250-250: Expected an expression
(invalid-syntax)
[warning] 251-251: Expected ,, found string
(invalid-syntax)
[warning] 251-251: Expected ,, found :
(invalid-syntax)
[warning] 255-255: Expected an expression
(invalid-syntax)
[warning] 255-255: Expected an expression
(invalid-syntax)
[warning] 256-256: Expected ,, found {
(invalid-syntax)
[warning] 256-256: Expected an expression
(invalid-syntax)
[warning] 256-256: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 256-256: Expected ,, found name
(invalid-syntax)
[warning] 256-256: Expected an expression
(invalid-syntax)
[warning] 257-257: Expected ,, found string
(invalid-syntax)
[warning] 257-257: Expected ,, found :
(invalid-syntax)
[warning] 262-262: Expected an expression
(invalid-syntax)
[warning] 262-262: Expected an expression
(invalid-syntax)
[warning] 263-263: Expected ,, found {
(invalid-syntax)
[warning] 263-263: Expected an expression
(invalid-syntax)
[warning] 263-263: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 263-263: Expected ,, found name
(invalid-syntax)
[warning] 263-263: Expected an expression
(invalid-syntax)
[warning] 264-264: Expected ,, found string
(invalid-syntax)
[warning] 264-264: Expected ,, found :
(invalid-syntax)
[warning] 268-268: Expected an expression
(invalid-syntax)
[warning] 268-268: Expected an expression
(invalid-syntax)
[warning] 269-269: Expected ,, found {
(invalid-syntax)
[warning] 269-269: Expected an expression
(invalid-syntax)
[warning] 269-269: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 269-269: Expected ,, found name
(invalid-syntax)
[warning] 269-269: Expected an expression
(invalid-syntax)
[warning] 270-270: Expected ,, found string
(invalid-syntax)
[warning] 270-270: Expected ,, found :
(invalid-syntax)
[warning] 276-276: Expected ,, found :
(invalid-syntax)
[warning] 284-284: Expected ,, found :
(invalid-syntax)
[warning] 290-290: Expected ,, found :
(invalid-syntax)
[warning] 296-296: Expected an expression
(invalid-syntax)
[warning] 296-296: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 296-296: Expected ,, found name
(invalid-syntax)
[warning] 296-296: Expected an expression
(invalid-syntax)
[warning] 297-297: Expected ,, found string
(invalid-syntax)
[warning] 297-297: Expected ,, found :
(invalid-syntax)
[warning] 303-303: Expected an expression
(invalid-syntax)
[warning] 303-303: Expected an expression
(invalid-syntax)
[warning] 304-304: Expected ,, found {
(invalid-syntax)
[warning] 304-304: Expected an expression
(invalid-syntax)
[warning] 304-304: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 304-304: Expected ,, found name
(invalid-syntax)
[warning] 304-304: Expected an expression
(invalid-syntax)
[warning] 305-305: Expected ,, found string
(invalid-syntax)
[warning] 305-305: Expected ,, found :
(invalid-syntax)
[warning] 310-310: Expected an expression
(invalid-syntax)
[warning] 310-310: Expected an expression
(invalid-syntax)
[warning] 311-311: Expected ,, found {
(invalid-syntax)
[warning] 311-311: Expected an expression
(invalid-syntax)
[warning] 311-311: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 311-311: Expected ,, found name
(invalid-syntax)
[warning] 311-311: Expected an expression
(invalid-syntax)
[warning] 312-312: Expected ,, found string
(invalid-syntax)
[warning] 312-312: Expected ,, found :
(invalid-syntax)
[warning] 317-317: Expected an expression
(invalid-syntax)
[warning] 317-317: Expected an expression
(invalid-syntax)
[warning] 318-318: Expected ,, found {
(invalid-syntax)
[warning] 318-318: Expected an expression
(invalid-syntax)
[warning] 318-318: Expected an expression
(invalid-syntax)
[warning] 319-319: Expected ,, found {
(invalid-syntax)
[warning] 319-319: Expected an expression
(invalid-syntax)
[warning] 319-319: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 319-319: Expected ,, found name
(invalid-syntax)
[warning] 319-319: Expected an expression
(invalid-syntax)
[warning] 320-320: Expected ,, found string
(invalid-syntax)
[warning] 320-320: Expected ,, found :
(invalid-syntax)
[warning] 324-324: Expected an expression
(invalid-syntax)
[warning] 324-324: Expected an expression
(invalid-syntax)
[warning] 327-327: Expected an expression
(invalid-syntax)
[warning] 327-327: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 327-327: Expected ,, found name
(invalid-syntax)
[warning] 327-327: Expected an expression
(invalid-syntax)
[warning] 329-329: Expected an expression
(invalid-syntax)
[warning] 329-329: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 329-329: Expected ,, found name
(invalid-syntax)
[warning] 329-329: Expected an expression
(invalid-syntax)
[warning] 331-331: Expected ,, found string
(invalid-syntax)
[warning] 331-331: Expected ,, found :
(invalid-syntax)
[warning] 332-332: Expected an expression
(invalid-syntax)
[warning] 332-332: Expected an expression
(invalid-syntax)
[warning] 333-333: Expected ,, found {
(invalid-syntax)
[warning] 333-333: Expected an expression
(invalid-syntax)
[warning] 333-333: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 333-333: Expected ,, found name
(invalid-syntax)
[warning] 333-333: Expected an expression
(invalid-syntax)
[warning] 335-335: Expected ,, found string
(invalid-syntax)
[warning] 335-335: Expected ,, found :
(invalid-syntax)
[warning] 336-336: Expected ,, found :
(invalid-syntax)
[warning] 339-339: Expected an expression
(invalid-syntax)
[warning] 339-339: Expected an expression
(invalid-syntax)
[warning] 340-340: Expected ,, found {
(invalid-syntax)
[warning] 340-340: Expected an expression
(invalid-syntax)
[warning] 340-340: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 340-340: Expected ,, found name
(invalid-syntax)
[warning] 340-340: Expected an expression
(invalid-syntax)
[warning] 342-342: Expected ,, found string
(invalid-syntax)
[warning] 342-342: Expected ,, found :
(invalid-syntax)
[warning] 343-343: Expected ,, found :
(invalid-syntax)
[warning] 346-346: Expected an expression
(invalid-syntax)
[warning] 346-346: Expected an expression
(invalid-syntax)
[warning] 349-349: Expected an expression
(invalid-syntax)
[warning] 349-349: Expected an expression
(invalid-syntax)
[warning] 378-378: Expected an expression
(invalid-syntax)
[warning] 378-378: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 378-378: Expected ,, found name
(invalid-syntax)
[warning] 378-378: Expected an expression
(invalid-syntax)
[warning] 379-379: Expected :, found string
(invalid-syntax)
[warning] 379-379: Expected ,, found :
(invalid-syntax)
[warning] 379-379: Expected :, found ,
(invalid-syntax)
[warning] 380-380: Expected an expression
(invalid-syntax)
[warning] 380-380: Expected an expression
(invalid-syntax)
[warning] 381-381: Expected :, found {
(invalid-syntax)
[warning] 381-381: Expected an expression
(invalid-syntax)
[warning] 381-381: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 381-381: Expected ,, found name
(invalid-syntax)
[warning] 381-381: Expected an expression
(invalid-syntax)
[warning] 382-382: Expected ,, found string
(invalid-syntax)
[warning] 383-383: Expected an expression
(invalid-syntax)
[warning] 383-383: Expected an expression
(invalid-syntax)
[warning] 384-384: Expected :, found {
(invalid-syntax)
[warning] 384-384: Expected an expression
(invalid-syntax)
[warning] 384-384: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 384-384: Expected ,, found name
(invalid-syntax)
[warning] 384-384: Expected an expression
(invalid-syntax)
[warning] 385-385: Expected ,, found string
(invalid-syntax)
[warning] 386-386: Expected an expression
(invalid-syntax)
[warning] 386-386: Expected an expression
(invalid-syntax)
[warning] 387-387: Expected :, found {
(invalid-syntax)
[warning] 387-387: Expected an expression
(invalid-syntax)
[warning] 387-387: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 387-387: Expected ,, found name
(invalid-syntax)
[warning] 387-387: Expected an expression
(invalid-syntax)
[warning] 388-388: Expected ,, found string
(invalid-syntax)
[warning] 389-389: Expected an expression
(invalid-syntax)
[warning] 389-389: Expected an expression
(invalid-syntax)
[warning] 390-390: Expected :, found {
(invalid-syntax)
[warning] 390-390: Expected an expression
(invalid-syntax)
[warning] 390-390: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 390-390: Expected ,, found name
(invalid-syntax)
[warning] 390-390: Expected an expression
(invalid-syntax)
[warning] 391-391: Expected ,, found string
(invalid-syntax)
[warning] 392-392: Expected an expression
(invalid-syntax)
[warning] 392-392: Expected an expression
(invalid-syntax)
[warning] 393-393: Expected :, found }
(invalid-syntax)
[warning] 396-396: Expected an expression
(invalid-syntax)
[warning] 396-396: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 396-396: Expected ,, found name
(invalid-syntax)
[warning] 396-396: Expected an expression
(invalid-syntax)
[warning] 398-398: Expected an expression
(invalid-syntax)
[warning] 398-398: Expected an expression
(invalid-syntax)
[warning] 418-418: Expected an expression
(invalid-syntax)
[warning] 418-418: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 418-418: Expected ,, found name
(invalid-syntax)
[warning] 418-418: Expected an expression
(invalid-syntax)
[warning] 420-420: Expected an expression
(invalid-syntax)
[warning] 420-420: Expected an expression
(invalid-syntax)
[warning] 432-432: Expected an expression
(invalid-syntax)
[warning] 432-432: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 432-432: Expected ,, found name
(invalid-syntax)
[warning] 432-432: Expected an expression
(invalid-syntax)
[warning] 434-434: Expected an expression
(invalid-syntax)
[warning] 434-434: Expected an expression
(invalid-syntax)
[warning] 440-440: Expected an expression
(invalid-syntax)
[warning] 440-440: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 440-440: Expected ,, found name
(invalid-syntax)
[warning] 440-440: Expected an expression
(invalid-syntax)
[warning] 442-442: Expected an expression
(invalid-syntax)
[warning] 442-442: Expected an identifier, but found a keyword else that cannot be used here
(invalid-syntax)
[warning] 442-442: Expected an expression
(invalid-syntax)
[warning] 444-444: Expected an expression
(invalid-syntax)
[warning] 444-444: Expected an expression
(invalid-syntax)
[warning] 495-495: Expected an expression
(invalid-syntax)
[warning] 495-495: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 495-495: Expected ,, found name
(invalid-syntax)
[warning] 495-495: Expected an expression
(invalid-syntax)
[warning] 496-496: Expected :, found string
(invalid-syntax)
[warning] 496-496: Expected ,, found :
(invalid-syntax)
[warning] 500-500: Expected :, found ,
(invalid-syntax)
[warning] 501-501: Expected an expression
(invalid-syntax)
[warning] 501-501: Expected an expression
(invalid-syntax)
[warning] 502-502: Expected :, found }
(invalid-syntax)
[warning] 509-509: Expected an expression
(invalid-syntax)
[warning] 509-509: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 509-509: Expected ,, found name
(invalid-syntax)
[warning] 509-509: Expected an expression
(invalid-syntax)
[warning] 510-510: Expected ,, found string
(invalid-syntax)
[warning] 511-511: Expected an expression
(invalid-syntax)
[warning] 511-511: Expected an expression
(invalid-syntax)
[warning] 534-534: Expected an expression
(invalid-syntax)
[warning] 534-534: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 534-534: Expected ,, found name
(invalid-syntax)
[warning] 534-534: Expected an expression
(invalid-syntax)
[warning] 536-536: Expected an expression
(invalid-syntax)
[warning] 536-536: Expected an expression
(invalid-syntax)
[warning] 538-538: Expected an expression
(invalid-syntax)
[warning] 538-538: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 538-538: Expected ,, found name
(invalid-syntax)
[warning] 538-538: Expected an expression
(invalid-syntax)
[warning] 550-550: Expected an expression
(invalid-syntax)
[warning] 550-550: Expected an expression
(invalid-syntax)
[warning] 552-552: Expected an expression
(invalid-syntax)
[warning] 552-552: Expected an identifier, but found a keyword for that cannot be used here
(invalid-syntax)
[warning] 552-552: Expected ,, found name
(invalid-syntax)
[warning] 552-552: Expected an expression
(invalid-syntax)
[warning] 553-553: Invalid assignment target
(invalid-syntax)
[warning] 553-553: Expected an expression
(invalid-syntax)
[warning] 553-553: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 553-553: Expected ,, found name
(invalid-syntax)
[warning] 553-553: Expected an expression
(invalid-syntax)
[warning] 553-553: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 553-553: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 553-553: Expected an expression
(invalid-syntax)
[warning] 553-553: Expected an identifier, but found a keyword else that cannot be used here
(invalid-syntax)
[warning] 553-553: Expected an expression
(invalid-syntax)
[warning] 553-553: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 553-553: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 553-553: Expected an expression
(invalid-syntax)
[warning] 553-553: Expected an expression
(invalid-syntax)
[warning] 554-554: Expected an expression
(invalid-syntax)
[warning] 554-554: Expected an expression
(invalid-syntax)
[warning] 556-556: Expected an expression
(invalid-syntax)
[warning] 556-556: Expected an identifier, but found a keyword for that cannot be used here
(invalid-syntax)
[warning] 556-556: Expected ,, found name
(invalid-syntax)
[warning] 556-556: Expected an expression
(invalid-syntax)
[warning] 558-558: Expected an expression
(invalid-syntax)
[warning] 558-558: Expected an expression
(invalid-syntax)
[warning] 560-560: Expected an expression
(invalid-syntax)
[warning] 560-560: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 560-560: Expected ,, found name
(invalid-syntax)
[warning] 560-560: Expected an expression
(invalid-syntax)
[warning] 568-568: Expected an expression
(invalid-syntax)
[warning] 568-568: Expected an expression
(invalid-syntax)
⏰ 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). (2)
- GitHub Check: CodeQL analysis (python)
- GitHub Check: Agent
🔇 Additional comments (30)
.flake8 (1)
1-11: LGTM — exclude list looks reasonable.pyproject.toml (1)
4-10: Keep Flake8 excludes consistent across configs.
.flake8excludestemplates/**, but[tool.flake8]does not (Line 5–10). If flake8 is run viaflake8-pyprojector another tool that readspyproject.toml, Jinja templates may still be linted, reintroducing the false positives this PR aims to avoid. Please align the lists or remove the duplicate config.🔧 Suggested alignment (option A: mirror .flake8)
[tool.flake8] exclude = [ ".venv", + ".venv/*", + "**/.venv/*", "venv", + "venv/*", + "**/venv/*", "__pycache__", ".git", + "templates/**", ]tasks/apt.yml (1)
1-169: LGTM — formatting-only change set.tasks/main.yml (1)
1-43: LGTM — no semantic changes detected.tasks/ssh.yml (1)
1-32: LGTM — formatting-only update.tasks/supervisor.yml (1)
1-72: LGTM — quoting modes is fine; no behavior change.meta/main.yml (1)
1-37: LGTM — formatting-only change set..github/FUNDING.yml (1)
1-16: LGTM!Adding the YAML document start marker (
---) is a good practice for YAML files and aligns with the PR's goal of fixing formatting issues detected by QA checks.tasks/django.yml (1)
1-237: LGTM on overall structure.The Django task file is well-organized with clear task names, appropriate use of handlers, tags, and conditionals. The logic flow for Django setup, migrations, and static file handling is correct.
run-qa-checks (1)
1-4: LGTM on the script logic.The simplified script correctly:
- Uses
set -eto exit immediately on any error- Runs the full QA check suite without skip flags
- Avoids error suppression constructs like
|| trueThis aligns well with the PR objective to ensure lint/QA failures are no longer silently ignored.
molecule/local/molecule.yml (1)
1-56: LGTM!The removal of the lint block from the Molecule configuration aligns with the PR objective to centralize lint checks in the
run-qa-checksscript. This ensures lint errors are properly enforced through the dedicated QA script rather than being potentially bypassed in the Molecule configuration.The platform definitions, provisioner settings, and verifier configuration are well-structured.
tasks/django_secret_key.yml (1)
1-27: LGTM!The file properly:
- Uses quoted mode values (
"0754","0600") which is the recommended practice for Ansible- Implements idempotent secret key generation with the
createsargument- Sets appropriately restrictive permissions (
0600) on the secret key file- Uses
changed_when: falsefor the read-only commanddocs/developer/installation.rst (1)
103-121: LGTM! Clear and helpful documentation addition.The new "How to Run QA checks" section provides valuable guidance for contributors. The instructions align with the simplified
run-qa-checksscript.One minor consideration:
ansible-lint --fixonly auto-fixes ansible-lint issues. If developers encounter yamllint or flake8 errors, they may need different remediation steps. You might optionally expand the troubleshooting guidance to mention this..ansible-lint (1)
1-9: Formatting-only change looks good.
No behavioral impact detected.vars/debian-13.yml (1)
1-7: Formatting-only change looks good.tasks/complete.yml (1)
1-14: Formatting-only change looks good.molecule/resources/requirements.yml (1)
1-8: Formatting-only change looks good.tasks/system.yml (1)
1-14: Formatting-only change looks good.molecule/resources/converge.yml (1)
1-42: Formatting-only change looks good..yamllint.yml (1)
1-36: LGTM: ruleset expansion looks consistent.
The added rules and ignore patterns are clear and cohesive with the goal of stricter YAML hygiene.molecule/default/molecule.yml (1)
1-36: LGTM: Molecule config simplification aligns with centralized QA.
Moving lint out and keeping platforms top-level is clean and consistent with a single QA entrypoint.templates/openwisp2/settings.py (1)
1-23: Linter exclusion for this file is already properly configured. The.flake8configuration explicitly excludestemplates/**(line 11), which covers this Jinja-templated settings module and prevents linting errors.molecule/resources/verify.yml (1)
50-54: This is test code that uses self-signed certificates intentionally generated during the test setup. In a Molecule CI environment with self-signed certificates,validate_certs: falseis necessary and standard practice—making it configurable would break the tests unnecessarily.Likely an incorrect or invalid review comment.
tasks/freeradius_eap.yml (1)
1-77: LGTM!The task file is well-structured with consistent use of:
- Quoted mode values (
"0644")- Proper handler notifications for freeradius restarts
- Appropriate tags on all tasks
- Block-level notify for the DH certificate generation group
.github/workflows/ci.yml (1)
52-67: Improved CI flow aligns with PR objectives.Good changes:
- QA checks no longer have
continue-on-error, ensuring failures are properly reported- Tests step now has explicit conditional
!cancelled() && steps.deps.conclusion == 'success'which ensures tests only run when dependencies are successfully installedThis correctly addresses the issue of QA/lint errors being silently ignored.
defaults/main.yml (2)
30-38: LGTM - Nested dictionary structure.The conversion to nested dictionaries is cleaner and allows natural dot notation access (e.g.,
openwisp2_database.engine,openwisp2_database.options.timeout). This is consistent with Ansible best practices for structured data.
1-236: Well-structured defaults file.The restructured YAML is consistent throughout:
- Database configurations use proper nested mappings
- Null values for optional settings are correctly represented
- All sections follow the same structural pattern
tasks/pip.yml (2)
117-129: Consider adding a condition for the copy task as well.The copy task at line 113-116 will always execute, but the pip install task at line 119-129 only runs when CORS is enabled. This leaves an orphaned requirements file when CORS is disabled.
However, looking more closely, line 117 already has the
whencondition on the copy task, so this is correctly implemented.
1-233: LGTM - Well-structured pip installation tasks.The tasks are properly implemented with:
- Consistent retry/delay patterns for network resilience
- Appropriate conditional execution based on feature flags
- Correct use of nested variable access (e.g.,
openwisp2_database.engine)- Proper idempotence tags where needed
tasks/freeradius.yml (1)
89-108: LGTM - FreeRADIUS package installation.The package installation tasks are well-structured with:
- Conditional installation based on SQL dialect
- Proper handler notifications for freeradius restarts
- Consistent
state: latestusage
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
.github/dependabot.yml
Outdated
| - package-ecosystem: "pip" # Check for Python package updates | ||
| directory: "/" # The root directory where the Ansible role is located | ||
| schedule: | ||
| interval: "monthly" # Check for updates weekly |
There was a problem hiding this comment.
Comment is inconsistent with configuration.
The comment says "Check for updates weekly" but the interval is set to "monthly". Consider updating the comment to match the actual configuration.
📝 Suggested fix
schedule:
- interval: "monthly" # Check for updates weekly
+ interval: "monthly" # Check for updates monthly📝 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.
| interval: "monthly" # Check for updates weekly | |
| interval: "monthly" # Check for updates monthly |
🤖 Prompt for AI Agents
In @.github/dependabot.yml at line 7, The inline comment is inconsistent with
the dependabot setting: the key interval: "monthly" does not match the comment
"Check for updates weekly"; update the comment to reflect the actual
configuration (change the comment to "Check for updates monthly") or, if weekly
updates are intended, change the interval value to "weekly" so the comment and
the interval: "monthly" setting for dependabot are consistent; edit the line
containing interval: "monthly" and its adjacent comment accordingly.
.github/dependabot.yml
Outdated
| - package-ecosystem: "github-actions" # Check for GitHub Actions updates | ||
| directory: "/" # The root directory where the Ansible role is located | ||
| schedule: | ||
| interval: "monthly" # Check for updates weekly |
There was a problem hiding this comment.
Same inconsistency here.
The comment says "weekly" but the interval is "monthly".
📝 Suggested fix
schedule:
- interval: "monthly" # Check for updates weekly
+ interval: "monthly" # Check for updates monthly📝 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.
| interval: "monthly" # Check for updates weekly | |
| schedule: | |
| interval: "monthly" # Check for updates monthly |
🤖 Prompt for AI Agents
In @.github/dependabot.yml at line 13, The configuration's interval value
doesn't match its inline comment: update the interval setting named "interval"
so it matches the intended cadence in the comment (change "monthly" to "weekly")
or adjust the comment to reflect the actual value; specifically edit the
"interval" entry in the dependabot configuration so the value and the comment
are consistent.
.github/workflows/ci.yml
Outdated
| - name: Set up Python ${{ matrix.python-version }} | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: "3.12" |
There was a problem hiding this comment.
Undefined matrix variable in step name.
The step name references matrix.python-version but the matrix only defines distro. This won't break the workflow but will display incorrectly in the GitHub Actions UI.
Proposed fix
- - name: Set up Python ${{ matrix.python-version }}
+ - name: Set up Python 3.12
uses: actions/setup-python@v6
with:
python-version: "3.12"📝 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.
| - name: Set up Python ${{ matrix.python-version }} | |
| uses: actions/setup-python@v6 | |
| with: | |
| python-version: "3.12" | |
| - name: Set up Python 3.12 | |
| uses: actions/setup-python@v6 | |
| with: | |
| python-version: "3.12" |
🧰 Tools
🪛 actionlint (1.7.10)
[error] 38-38: property "python-version" is not defined in object type {distro: string}
(expression)
🤖 Prompt for AI Agents
In @.github/workflows/ci.yml around lines 38 - 41, The step name uses an
undefined matrix variable "matrix.python-version" which should be corrected to
match the defined matrix value; update the step name "Set up Python ${{
matrix.python-version }}" to reference the actual matrix key (e.g., "Set up
Python ${{ matrix.distro }}" or simply "Set up Python") or alternatively add a
"python-version" entry to the matrix so the interpolation matches; locate the
workflow step with the name string and make the name and matrix keys consistent.
handlers/main.yml
Outdated
| - name: Migrate timeseries database | ||
| become: true | ||
| become_user: "{{ www_user }}" | ||
| community.general.django_manage: | ||
| app_path: "{{ openwisp2_path }}" | ||
| command: migrate_timeseries | ||
| virtualenv: "{{ virtualenv_path }}" | ||
| when: openwisp2_monitoring |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all Ansible requirements files
fd -e yml -e yaml | grep -E '(requirements|collections)' | head -20Repository: openwisp/ansible-openwisp2
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Search for community.general in the entire repository
rg 'community\.general' -A 1 -B 1Repository: openwisp/ansible-openwisp2
Length of output: 791
🏁 Script executed:
#!/bin/bash
# Check content of molecule resources requirements file
cat molecule/resources/requirements.ymlRepository: openwisp/ansible-openwisp2
Length of output: 284
🏁 Script executed:
#!/bin/bash
# Look for requirements files at root and common locations
fd -e yml -e yaml -path '*/requirements*' --max-results 10Repository: openwisp/ansible-openwisp2
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Check if there's a requirements.yml at the root
if [ -f requirements.yml ]; then
cat requirements.yml
else
echo "No requirements.yml at root"
fiRepository: openwisp/ansible-openwisp2
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for any collections configuration
rg 'collections:' -B 2 -A 5 --type yamlRepository: openwisp/ansible-openwisp2
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check if there's an ansible.cfg or galaxy.yml
ls -la | grep -E '(ansible\.cfg|galaxy\.yml|collections)'Repository: openwisp/ansible-openwisp2
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check root directory for any requirements or config files
find . -maxdepth 1 -type f \( -name '*.yml' -o -name '*.yaml' -o -name 'requirements*' -o -name 'ansible.cfg' \)Repository: openwisp/ansible-openwisp2
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Check for any file with 'requirements' or 'collections' in its name
find . -type f \( -name '*requirements*' -o -name '*collections*' \) | grep -v '.git' | head -20Repository: openwisp/ansible-openwisp2
Length of output: 335
🏁 Script executed:
#!/bin/bash
# Check the requirements-test.txt to see if it has Ansible collections info
cat requirements-test.txtRepository: openwisp/ansible-openwisp2
Length of output: 183
🏁 Script executed:
#!/bin/bash
# Check if there's a galaxy.yml or any collections declaration
find . -name 'galaxy.yml' -o -name '*collections*' -type f | head -10Repository: openwisp/ansible-openwisp2
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Let's look at the complete structure of molecule directory
ls -la molecule/Repository: openwisp/ansible-openwisp2
Length of output: 351
🏁 Script executed:
#!/bin/bash
# Check if molecule/resources/requirements.yml should include collections
head -30 molecule/resources/requirements.ymlRepository: openwisp/ansible-openwisp2
Length of output: 284
🏁 Script executed:
#!/bin/bash
# Check the README to understand how this repository is meant to be used
if [ -f README.md ]; then
head -100 README.md
else
echo "No README.md found"
fiRepository: openwisp/ansible-openwisp2
Length of output: 995
🏁 Script executed:
#!/bin/bash
# Check the user documentation that mentioned community.general
cat docs/user/quickstart.rst | head -50Repository: openwisp/ansible-openwisp2
Length of output: 1624
🏁 Script executed:
#!/bin/bash
# Continue reading the quickstart to find the collections section
cat docs/user/quickstart.rst | grep -A 20 "community.general"Repository: openwisp/ansible-openwisp2
Length of output: 1387
🏁 Script executed:
#!/bin/bash
# Check if there's a main.yml playbook or similar that might have collection requirements
find . -name 'main.yml' -o -name 'site.yml' -o -name 'playbook.yml' | head -10Repository: openwisp/ansible-openwisp2
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Check if there's a collections requirement in meta/main.yml or similar
find . -path '*/meta/*' -name '*.yml' -o -path '*/meta/*' -name '*.yaml'Repository: openwisp/ansible-openwisp2
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Check meta/main.yml for collections declaration
cat meta/main.ymlRepository: openwisp/ansible-openwisp2
Length of output: 1133
Add community.general to the role's collection requirements.
The community.general collection is used by the django_manage module but is not declared in a machine-readable requirements format within the role. While the user documentation mentions manual installation via ansible-galaxy collection install "community.general:>=3.6.0", users might miss this step. Add a collections declaration to meta/main.yml under galaxy_info to ensure the requirement is enforced:
galaxy_info:
collections:
- name: community.general
version: ">=3.6.0"
- name: ansible.posix
This prevents runtime failures when the handler is executed.
🤖 Prompt for AI Agents
In `@handlers/main.yml` around lines 48 - 55, Add a machine-readable collection
requirement so the play using community.general.django_manage always has the
collection installed: update the role metadata by adding a
galaxy_info.collections entry that declares community.general with version
">=3.6.0" (and also add ansible.posix) under the role's meta configuration so
the dependency is enforced for handlers that call the django_manage module;
ensure the collections list includes the names and version constraint exactly as
specified.
README.md
Outdated
| [](https://github.com/openwisp/ansible-openwisp2/actions/workflows/ci.yml) | ||
| [](https://galaxy.ansible.com/ui/standalone/roles/openwisp/openwisp2/) | ||
| [](https://gitter.im/openwisp/general) |
There was a problem hiding this comment.
Use HTTPS for the Galaxy badge image URL.
The http badge can trigger mixed‑content warnings and allows MITM on the image. Switch to https for consistency with the other badges.
✅ Suggested fix
-[](https://galaxy.ansible.com/ui/standalone/roles/openwisp/openwisp2/)
+[](https://galaxy.ansible.com/ui/standalone/roles/openwisp/openwisp2/)📝 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.
| [](https://github.com/openwisp/ansible-openwisp2/actions/workflows/ci.yml) | |
| [](https://galaxy.ansible.com/ui/standalone/roles/openwisp/openwisp2/) | |
| [](https://gitter.im/openwisp/general) | |
| [](https://github.com/openwisp/ansible-openwisp2/actions/workflows/ci.yml) | |
| [](https://galaxy.ansible.com/ui/standalone/roles/openwisp/openwisp2/) | |
| [](https://gitter.im/openwisp/general) |
🤖 Prompt for AI Agents
In `@README.md` around lines 3 - 5, Replace the insecure HTTP Galaxy badge image
URL with HTTPS: find the markdown line starting with the "[![Galaxy]" badge (the
image URL currently
"http://img.shields.io/badge/galaxy-openwisp.openwisp2-blue.svg?style=flat-square")
and update it to use "https://", ensuring the badge image is loaded securely
like the other badges.
tasks/cron.yml
Outdated
| cron: | ||
| name: "Update toplogies" | ||
| day: "{{ openwisp2_topology_update_frequency.day }}" |
There was a problem hiding this comment.
Fix typo in cron name to avoid confusing crontab entries.
“toplogies” should be “topologies”.
📝 Proposed fix
- name: "Update toplogies"
+ name: "Update topologies"📝 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.
| cron: | |
| name: "Update toplogies" | |
| day: "{{ openwisp2_topology_update_frequency.day }}" | |
| cron: | |
| name: "Update topologies" | |
| day: "{{ openwisp2_topology_update_frequency.day }}" |
🤖 Prompt for AI Agents
In `@tasks/cron.yml` around lines 5 - 7, Update the typo in the cron job name:
change the value of the cron.name string "Update toplogies" to "Update
topologies" so the cron entry is correctly spelled; edit the cron mapping where
the name key is defined (cron -> name) to use the corrected spelling.
tasks/freeradius.yml
Outdated
| - name: Add Network RADIUS PGP public key | ||
| get_url: | ||
| url: https://packages.networkradius.com/pgp/packages%40networkradius.com | ||
| dest: /etc/apt/trusted.gpg.d/packages.networkradius.com.asc | ||
| checksum: sha256:f9327befd8efb3fddd710ed8b4ed50d394cc57a598ca8c3ccc2dc3234a4d774a | ||
| ignore_errors: true | ||
| retries: 5 | ||
| delay: 10 | ||
| register: result | ||
| until: result is success |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "packages.networkradius.com" --type yaml -A5 -B2Repository: openwisp/ansible-openwisp2
Length of output: 3759
🏁 Script executed:
# Get the full context of the freeradius.yml file around the mentioned lines
fd "freeradius.yml" --type f | head -5Repository: openwisp/ansible-openwisp2
Length of output: 90
🏁 Script executed:
# Check the broader context of the freeradius.yml file
wc -l tasks/freeradius.ymlRepository: openwisp/ansible-openwisp2
Length of output: 94
🏁 Script executed:
# Get lines 74 onwards to see what happens after the PGP key download
sed -n '74,$p' tasks/freeradius.yml | head -40Repository: openwisp/ansible-openwisp2
Length of output: 1111
🏁 Script executed:
# Let's also see the full structure around lines 20-100 to understand the flow
sed -n '20,100p' tasks/freeradius.ymlRepository: openwisp/ansible-openwisp2
Length of output: 3475
Remove ignore_errors: true from the PGP key download task or add explicit failure handling.
The combination of ignore_errors: true with until: result is success creates a problematic pattern: the task will retry up to 5 times, but if all retries fail, the error is silenced rather than causing the playbook to fail. Subsequent apt update and apt install operations depend on this key file (referenced via signed-by= in repository configurations at lines 27, 33, 43, 54, 65), so silently continuing without the key will cause those operations to fail anyway—defeating the purpose of error suppression while masking the root cause.
Either remove ignore_errors: true to properly fail if the key cannot be downloaded after retries, or add an explicit assert task to verify the file exists before proceeding to apt operations.
🤖 Prompt for AI Agents
In `@tasks/freeradius.yml` around lines 74 - 83, The task "Add Network RADIUS PGP
public key" currently uses ignore_errors: true while registering result and
using until: result is success, which masks failures; remove ignore_errors: true
so the playbook will fail if the get_url task (register: result, until: result
is success, retries/delay) cannot download the key, or alternatively keep
ignore_errors but add an explicit verification step after the task (e.g., an
assert or stat checking the presence of
/etc/apt/trusted.gpg.d/packages.networkradius.com.asc and failing the playbook
if not found) before any apt update/install tasks that depend on the signed-by
key.
tasks/freeradius.yml
Outdated
| - name: Copy configuration for WPA Enterprise TTLS | ||
| include_tasks: tasks/freeradius_eap.yml | ||
| loop: "{{ freeradius_eap_orgs }}" | ||
| loop_control: | ||
| loop_var: org | ||
| when: freeradius_eap_orgs | length > 0 | ||
| tags: [freeradius_eap] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the freeradius_eap.yml file
echo "=== Searching for freeradius_eap.yml ==="
fd -t f "freeradius_eap.yml"
# Check the directory structure around tasks/freeradius.yml
echo -e "\n=== Directory structure ==="
fd -t f "freeradius" --type f | head -20
# Check all include_tasks usages in YAML files
echo -e "\n=== include_tasks patterns in codebase ==="
rg "include_tasks:" --type yaml -B1 -A1 | head -50Repository: openwisp/ansible-openwisp2
Length of output: 448
🏁 Script executed:
#!/bin/bash
# Check the full context of the include_tasks and any variables
echo "=== Full context of freeradius.yml around line 195-201 ==="
sed -n '190,210p' tasks/freeradius.yml
# Look for all include_tasks in the entire codebase (not just in tasks/)
echo -e "\n=== All include_tasks usages in YAML files ==="
rg "include_tasks:" --type yaml -B1 -A2
# Check if there's a role structure
echo -e "\n=== Check for role structure ==="
ls -la | head -20
cat meta/main.yml 2>/dev/null || echo "No meta/main.yml found"Repository: openwisp/ansible-openwisp2
Length of output: 3026
🏁 Script executed:
#!/bin/bash
# Look for test files or CI/CD that might validate this
echo "=== Looking for tests or CI validation ==="
find . -name "*.yml" -o -name "*.yaml" | grep -E "(test|spec|molecule)" | head -20
# Check if there are any error logs or issues mentioning this path
echo -e "\n=== Check CHANGES.md for any recent fixes ==="
grep -i "include_tasks\|freeradius_eap" CHANGES.md
# Look for any documentation about path resolution
echo -e "\n=== Check if role is being tested somewhere ==="
find . -type f -name "*.yml" | xargs grep -l "openwisp2" | head -10Repository: openwisp/ansible-openwisp2
Length of output: 728
🏁 Script executed:
#!/bin/bash
# Check the molecule converge file
echo "=== Molecule converge.yml ==="
cat molecule/resources/converge.yml
# Check if freeradius_eap_orgs is set in tests
echo -e "\n=== Check for freeradius_eap_orgs in test files ==="
rg "freeradius_eap_orgs" --type yamlRepository: openwisp/ansible-openwisp2
Length of output: 1400
🏁 Script executed:
#!/bin/bash
# Check if there's a CI workflow that runs these tests
echo "=== Check CI workflow ==="
cat .github/workflows/ci.yml | head -100
# Look for any recent commits or issues about this path
echo -e "\n=== Git log for freeradius.yml ==="
git log --oneline tasks/freeradius.yml | head -10
# Check if the path works by looking at the actual file
echo -e "\n=== Verify include_tasks behavior in Ansible docs ==="
echo "According to Ansible documentation, include_tasks paths in roles are relative to the role's tasks/ directory"
# Let's check if there are any reports or issues about this
echo -e "\n=== Search for any error handling or workarounds ==="
rg "tasks/freeradius_eap" --type yaml -B2 -A2Repository: openwisp/ansible-openwisp2
Length of output: 2862
🌐 Web query:
Ansible include_tasks path resolution relative to role tasks directory
💡 Result:
include_tasks resolves its file as a local path on the control node using Ansible’s task search path (“search magic”).
What “relative” means inside a role
-
If you are in a role task file (for example
roles/myrole/tasks/main.yml) and you do:- ansible.builtin.include_tasks: other.yml
Ansible will find
roles/myrole/tasks/other.yml(role context has highest precedence). (docs.ansible.com) -
If you include from a task file in a subdirectory (for example
roles/myrole/tasks/setup/main.yml) and do:- ansible.builtin.include_tasks: common.yml
it will first try relative to the including task file’s directory (so
roles/myrole/tasks/setup/common.yml), then fall back through the stacked contexts. (docs.ansible.com)
Parent-role / caller-role behavior (often the surprise)
Ansible only treats another role as a “parent role” in the search stack if the current role was entered via include_role, import_role, or role dependencies—not merely because the role was listed under roles: in the play. This is a common reason a “callback.yml in the parent role” isn’t found. (docs.ansible.com)
If you want it unambiguous
Use an explicit path anchored to the current role:
- ansible.builtin.include_tasks: "{{ role_path }}/tasks/other.yml"(role_path points at the role root, so you add /tasks/... yourself.)
Debugging
Check ansible_search_path (and/or run with -vvvvv) to see exactly where Ansible searched. (docs.ansible.com)
Citations:
- 1: https://docs.ansible.com/ansible/latest/playbook_guide/playbook_pathing.html?utm_source=openai
- 2: https://docs.ansible.com/ansible/latest/playbook_guide/playbook_pathing.html?utm_source=openai
- 3: https://docs.ansible.com/ansible/latest/playbook_guide/playbook_pathing.html?utm_source=openai
- 4: https://docs.ansible.com/ansible/latest/playbook_guide/playbook_pathing.html?utm_source=openai
Simplify the include_tasks path to idiomatic form.
When including tasks from the same role directory, use just the filename:
include_tasks: freeradius_eap.ymlIn Ansible roles, include_tasks paths within the tasks/ directory should reference only the filename, not the directory prefix. Using tasks/freeradius_eap.yml is non-idiomatic and relies on fallback search path resolution rather than explicit pathing.
🤖 Prompt for AI Agents
In `@tasks/freeradius.yml` around lines 195 - 201, Replace the non-idiomatic
include_tasks path "tasks/freeradius_eap.yml" with the filename only
"freeradius_eap.yml" in the task that loops over freeradius_eap_orgs (the task
with include_tasks, loop: "{{ freeradius_eap_orgs }}", loop_control.loop_var:
org and tags: [freeradius_eap]) so the role includes the task file from the
role’s tasks/ directory in the standard Ansible way; keep the loop, loop_control
and when condition unchanged.
tasks/nginx.yml
Outdated
| - name: Create SSL cert if not exists yet | ||
| command: > | ||
| openssl req -new -nodes -x509 \ | ||
| -subj "/C={{ openwisp2_ssl_country }}/ST={{ openwisp2_ssl_state }} \ | ||
| /L={{ openwisp2_ssl_locality }}/O={{ openwisp2_ssl_organization }} \ | ||
| /CN={{ openwisp2_ssl_common_name }}" \ | ||
| -days 3650 \ | ||
| -keyout {{ openwisp2_ssl_key }} \ | ||
| -out {{ openwisp2_ssl_cert }} \ | ||
| -extensions v3_ca creates={{ openwisp2_ssl_cert }} | ||
| notify: Restart nginx |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "nginx.yml" -type f | head -20Repository: openwisp/ansible-openwisp2
Length of output: 87
🏁 Script executed:
git ls-files | grep -i nginxRepository: openwisp/ansible-openwisp2
Length of output: 215
🏁 Script executed:
sed -n '26,36p' tasks/nginx.ymlRepository: openwisp/ansible-openwisp2
Length of output: 543
🏁 Script executed:
head -40 tasks/nginx.yml | tail -20Repository: openwisp/ansible-openwisp2
Length of output: 748
🌐 Web query:
Ansible command module creates parameter syntax idempotency
💡 Result:
The ansible.builtin.command module is not inherently idempotent (it will run every time). To make it effectively idempotent, use creates and/or removes guards. Ansible checks these paths on the remote host before deciding whether to run the command. [1][2]
creates (skip if file already exists)
- Meaning: Run the command only if the file/glob does not exist.
- Order:
createsis checked beforeremoves. [1]
- name: Initialize app (only once)
ansible.builtin.command:
cmd: /usr/local/bin/init_app --db
creates: /var/lib/myapp/.initializedremoves (run only if file exists)
- Meaning: Run the command only if the file/glob does exist. [1]
- name: Cleanup legacy file (only if present)
ansible.builtin.command:
cmd: /usr/local/bin/cleanup_legacy
removes: /opt/legacy/.installed“Inline” syntax (legacy style) also works
Docs note you can place these “after the command” in free-form style: [1][2]
- ansible.builtin.command: /usr/local/bin/init_app --db creates=/var/lib/myapp/.initializedCheck mode behavior
In --check mode, command can only predict changed vs ok when creates or removes is provided; otherwise the task is skipped in check mode. [1]
Tip: Prefer a purpose-built idempotent module (e.g., file, package, user, etc.) when one exists; use command with creates/removes mainly as a last resort. [1]
Sources: Ansible command module docs. [1][2]
🏁 Script executed:
python3 << 'EOF'
# Let's understand how Ansible command module parses the command string with creates
# The 'command' module in Ansible has these key parameters:
# - cmd/command: the command to execute
# - creates: if this file exists, the command is skipped
# - removes: if this file doesn't exist, the command is skipped
# - chdir, env, stdin, etc.
# The issue is that when you write:
# command: >
# openssl req ... -extensions v3_ca creates={{ openwisp2_ssl_cert }}
#
# Ansible treats everything after "command:" as the actual shell command string
# So "creates=..." becomes part of the openssl command itself, not an Ansible module argument
# According to Ansible docs, creates should be at the same indentation level as 'command:'
print("Issue Verification:")
print("==================")
print("✓ The 'creates' parameter MUST be a module argument (at same level as 'command:')")
print("✓ When appended to the command string, it's passed literally to openssl")
print("✓ This breaks idempotency because Ansible won't check file existence")
print("\nCorrect syntax should be:")
print("""
- name: Create SSL cert if not exists yet
command: >
openssl req -new -nodes -x509 \\
-subj "..." \\
-days 3650 \\
-keyout {{ openwisp2_ssl_key }} \\
-out {{ openwisp2_ssl_cert }} \\
-extensions v3_ca
args:
creates: "{{ openwisp2_ssl_cert }}"
notify: Restart nginx
""")
EOFRepository: openwisp/ansible-openwisp2
Length of output: 654
Fix creates usage to make SSL cert generation idempotent.
The creates={{ openwisp2_ssl_cert }} is incorrectly embedded in the openssl command string instead of being a separate module argument. Move it to module args so Ansible will skip the task if the certificate file already exists.
✅ Proposed fix
- name: Create SSL cert if not exists yet
command: >
openssl req -new -nodes -x509 \
-subj "/C={{ openwisp2_ssl_country }}/ST={{ openwisp2_ssl_state }} \
/L={{ openwisp2_ssl_locality }}/O={{ openwisp2_ssl_organization }} \
/CN={{ openwisp2_ssl_common_name }}" \
-days 3650 \
-keyout {{ openwisp2_ssl_key }} \
-out {{ openwisp2_ssl_cert }} \
- -extensions v3_ca creates={{ openwisp2_ssl_cert }}
+ -extensions v3_ca
+ args:
+ creates: "{{ openwisp2_ssl_cert }}"
notify: Restart nginx🤖 Prompt for AI Agents
In `@tasks/nginx.yml` around lines 26 - 36, The task "Create SSL cert if not
exists yet" embeds creates={{ openwisp2_ssl_cert }} inside the openssl command
string, so Ansible won't treat it as the command module's creates argument;
remove the trailing "creates=..." from the command value and add a separate
top-level creates: "{{ openwisp2_ssl_cert }}" key under the same task (keeping
the command string intact and the notify: Restart nginx), so the command module
will skip execution when the certificate file exists.
The previous version was raising error when using ansible-lint.
Varadraj75
force-pushed
the
fix-lint-errors-485-followup
branch
from
59688e7 to
2a24864
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tasks/django.yml (3)
169-178:⚠️ Potential issue | 🟠 MajorAvoid masking copy errors with failed_when: false.
This suppresses real failures (permissions, disk full, etc.). Since you already
statthe source, it’s safer to conditionally skip instead of ignoring errors.✅ Suggested fix
- name: Copy static files become: true copy: src: ow2_static/ dest: "{{ openwisp2_path }}/static_custom" owner: "{{ www_user }}" group: "{{ www_group }}" mode: "0640" - failed_when: false + when: ow2_static_dir.stat.exists tags: - openwisp2_theme
122-138:⚠️ Potential issue | 🟠 MajorAdd community.general collection dependency to role metadata.
The role uses
community.general.django_managein multiple tasks but does not declare the collection dependency. Add the collection to thedependenciessection inmeta/main.ymlor create arequirements.ymlfile at the role root level to ensure the collection is installed before the role runs.Affected tasks: "Run geocoding check" (line ~122), "Migrate" (line ~142), "Collectstatic" (line ~215), "Compilemessages" (line ~230).
181-191:⚠️ Potential issue | 🟡 MinorUse
stdoutinstead ofoutfor the field name.The
community.general.django_managemodule returns standard Ansible common return values likestdout,stderr, andstdout_lines. The current code referencescollectstatic_output.out, which does not exist; this breaks idempotence reporting. Change tocollectstatic_output.stdout.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.ansible-lint.flake8.github/FUNDING.yml.github/dependabot.yml.github/workflows/ci.yml.yamllint.ymldefaults/main.ymldocs/developer/installation.rsthandlers/main.ymlmolecule/default/molecule.ymlmolecule/local/molecule.ymlmolecule/resources/converge.ymlmolecule/resources/requirements.ymlmolecule/resources/verify.ymlpyproject.tomlrun-qa-checkstasks/apt.ymltasks/complete.ymltasks/cron.ymltasks/django.ymltasks/django_secret_key.ymltasks/freeradius.ymltasks/freeradius_eap.ymltasks/main.ymltasks/nginx.ymltasks/pip.ymltasks/ssh.ymltasks/supervisor.ymltasks/system.ymlvars/debian-13.yml
💤 Files with no reviewable changes (9)
- molecule/default/molecule.yml
- molecule/local/molecule.yml
- tasks/system.yml
- tasks/pip.yml
- tasks/ssh.yml
- tasks/main.yml
- molecule/resources/requirements.yml
- tasks/cron.yml
- tasks/complete.yml
✅ Files skipped from review due to trivial changes (3)
- docs/developer/installation.rst
- .ansible-lint
- vars/debian-13.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- tasks/apt.yml
- .github/FUNDING.yml
- .yamllint.yml
- tasks/supervisor.yml
- .flake8
- tasks/nginx.yml
- run-qa-checks
- molecule/resources/verify.yml
- molecule/resources/converge.yml
- handlers/main.yml
⏰ 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). (4)
- GitHub Check: Build debian13
- GitHub Check: Build debian12
- GitHub Check: Build ubuntu2404
- GitHub Check: Build ubuntu2204
🔇 Additional comments (16)
.github/workflows/ci.yml (4)
5-5: LGTM!The inline yamllint disable comment is the correct approach to suppress the false positive on the
on:key being flagged as a truthy value.
43-44: LGTM!Adding the
id: depsenables proper step referencing for the conditional check in the Tests step.
56-57: LGTM!The conditional
!cancelled() && steps.deps.conclusion == 'success'ensures Tests run only when dependencies installed successfully while still allowing QA checks to fail independently. This aligns with the PR objective of making CI failures real and actionable.
64-65: LGTM!Quoting the environment variable values (
"1") is good YAML hygiene—it ensures the values are treated as strings rather than potentially being interpreted as integers.tasks/django_secret_key.yml (2)
2-6: Consistent quoted mode on copy task looks good.
21-24: Quoted mode on secret key file is correct.pyproject.toml (1)
4-10: Flake8 exclude block is clear and standard.tasks/freeradius_eap.yml (2)
11-13: Notify/tag placement on DH generation block is fine.
25-68: Quoted mode values are consistent across templates.tasks/django.yml (1)
2-115: Mode string normalization is consistent.Also applies to: 220-225
tasks/freeradius.yml (2)
2-40: Jammy gating and repo setup block look good.
110-189: Mode string quoting is consistent across FreeRADIUS templates.defaults/main.yml (4)
130-163: Structured nginx CSP/gzip config is clean and readable.
188-205: freeradius_db_map restructure looks consistent.
223-226: CORS config mapped cleanly under openwisp2_django_cors.
30-112: No downstream compatibility issues detected.All templates and tasks correctly reference the restructured variables using the new nested format. Verified:
openwisp2_database.engine,.nameaccessed correctly in tasks/django.yml, tasks/pip.yml, tasks/apt.ymlopenwisp2_topology_update_frequency.day,.hour,.minuteaccessed correctly in tasks/cron.ymlopenwisp2_timeseries_database.backend,.user,.password,.name,.host,.portaccessed correctly in templates/openwisp2/settings.py- Celery pool/autoscale variables referenced as flat scalars in all supervisor templates
No legacy access patterns found. Migration is complete and consistent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
3 participants
Checklist
Reference to Existing Issue
Closes #485 .
Continuation of #571
Description of Changes
This PR continues and completes the work started in #571 by @ssmadhavan006, addressing the original goal of issue #485: ensuring QA and lint checks are not ignored and that CI failures are real and actionable.
The branch is based on the commits from #571, with additional fixes applied on top to resolve remaining QA issues and align local behaviour with CI expectations.
Key changes
run-qa-checksproperly fails on QA, yaml-lint, and ansible-lint errors|| true)All CI checks are now green, and lint/QA failures are no longer silently ignored.
Screenshot
Screenshot showing local
run-qa-checksexecution with QA and documentation lint checksproperly enforced and failures correctly reported (RST files reported when applicable).