Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix detection of play roles vars missing prefix #3765

Merged
merged 4 commits into from
Sep 28, 2023
Merged

Fix detection of play roles vars missing prefix #3765

merged 4 commits into from
Sep 28, 2023

Conversation

cavcrosby
Copy link
Contributor

Fixes #3668.

@cavcrosby cavcrosby requested a review from a team as a code owner September 22, 2023 13:23
@cavcrosby cavcrosby temporarily deployed to ack September 22, 2023 13:23 — with GitHub Actions Inactive
cavcrosby
cavcrosby commented Sep 22, 2023
View reviewed changes
src/ansiblelint/constants.py
@@ -126,6 +126,36 @@ def main():
"pre_tasks",
"post_tasks",
]
PLAYBOOK_ROLE_KEYWORDS = [
Copy link
Contributor Author

@cavcrosby cavcrosby Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I derived this list of playbook role keywords from here https://docs.ansible.com/ansible/latest/reference_appendices/playbooks_keywords.html#role.

cavcrosby
cavcrosby commented Sep 22, 2023
View reviewed changes
src/ansiblelint/rules/var_naming.py
results = Runner(
Lintable("examples/playbooks/role_vars_prefix_detection.yml"),
rules=default_rules_collection,
exclude_paths=["examples/roles/role_vars_prefix_detection"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exclude_paths was set because otherwise the runner picks up the violations from the role too, I presume this happens because the playbook invokes said role.

cavcrosby
cavcrosby commented Sep 22, 2023
View reviewed changes
examples/playbooks/role_vars_prefix_detection.yml
role_vars_prefix_detection_var1: val1

- role: role_vars_prefix_detection
vars:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side blurb, I would think one would want to put role variables under the vars key instead of mixing them in the same dictionary as the role name. This would prevent confusion with a playbook's role keywords.

@ssbarnea ssbarnea temporarily deployed to ack September 28, 2023 11:41 — with GitHub Actions Inactive
@ssbarnea ssbarnea temporarily deployed to ack September 28, 2023 11:44 — with GitHub Actions Inactive
@ssbarnea ssbarnea temporarily deployed to ack September 28, 2023 12:08 — with GitHub Actions Inactive
ssbarnea
ssbarnea approved these changes Sep 28, 2023
View reviewed changes
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me but I want another pair of eyes on it, in case I missed something, especially as I almost never user roles: blocks myself, as I prefer to use them inside tasks.

@ssbarnea ssbarnea merged commit 9558c92 into ansible:main Sep 28, 2023
21 checks passed
@ssbarnea ssbarnea added the bug label Sep 28, 2023
@ssbarnea ssbarnea temporarily deployed to ack September 28, 2023 15:40 — with GitHub Actions Inactive
@cavcrosby cavcrosby deleted the fix-play-roles-vars-prefix branch September 28, 2023 20:54
nrdufour added a commit to nrdufour/home-ops that referenced this pull request Oct 3, 2023
@nrdufour
chore(deps): update dependency ansible-lint to v6.20.1 (#120)
9dbcd76 
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [ansible-lint](https://github.com/ansible/ansible-lint) ([changelog](https://github.com/ansible/ansible-lint/releases)) | patch | `==6.20.0` -> `==6.20.1` |

---

### Release Notes

<details>
<summary>ansible/ansible-lint (ansible-lint)</summary>

### [`v6.20.1`](https://github.com/ansible/ansible-lint/releases/tag/v6.20.1)

[Compare Source](ansible/ansible-lint@v6.20.0...v6.20.1)

#### Bugfixes

-   Fix detection of play roles vars missing prefix ([#&#8203;3765](ansible/ansible-lint#3765)) [@&#8203;cavcrosby](https://github.com/cavcrosby)
-   Prune reported errors after autofix ([#&#8203;3774](ansible/ansible-lint#3774)) [@&#8203;cidrblock](https://github.com/cidrblock)
-   `--fix` will now default to `all` ([#&#8203;3777](ansible/ansible-lint#3777)) [@&#8203;ajinkyau](https://github.com/ajinkyau)
-   Avoid false error caused by TypeError with jinja rule ([#&#8203;3773](ansible/ansible-lint#3773)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Sanity Rule Ignore Additions and Updates ([#&#8203;3766](ansible/ansible-lint#3766)) [@&#8203;alisonlhart](https://github.com/alisonlhart)
-   Update rulebook schema (name is now required) ([#&#8203;3776](ansible/ansible-lint#3776)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Expose rules that have autofix capability in docs ([#&#8203;3770](ansible/ansible-lint#3770)) [@&#8203;shatakshiiii](https://github.com/shatakshiiii)
-   List all the fixable rules in CLI via `--list-rules` ([#&#8203;3737](ansible/ansible-lint#3737)) [@&#8203;shatakshiiii](https://github.com/shatakshiiii)
-   Include level in SARIF results ([#&#8203;3758](ansible/ansible-lint#3758)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Meta runtime supported version update ([#&#8203;3755](ansible/ansible-lint#3755)) [@&#8203;alisonlhart](https://github.com/alisonlhart)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4wLjEiLCJ1cGRhdGVkSW5WZXIiOiIzNy4wLjEiLCJ0YXJnZXRCcmFuY2giOiJtYWluIn0=-->

Reviewed-on: https://git.home/nrdufour/home-ops/pulls/120
Co-authored-by: Renovate <renovate@ptinem.io>
Co-committed-by: Renovate <renovate@ptinem.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssbarnea ssbarnea ssbarnea approved these changes

@cidrblock cidrblock cidrblock approved these changes

@audgirka audgirka Awaiting requested review from audgirka audgirka is a code owner automatically assigned from ansible/devtools

@priyamsahoo priyamsahoo Awaiting requested review from priyamsahoo priyamsahoo is a code owner automatically assigned from ansible/devtools

@shatakshiiii shatakshiiii Awaiting requested review from shatakshiiii shatakshiiii is a code owner automatically assigned from ansible/devtools

Assignees
No one assigned
Labels
bug
Projects
🧰 devtools project board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

no-role-prefix do not work in role
3 participants
@cavcrosby @ssbarnea @cidrblock