Skip to content

Fix warning consistency and internal clobbers - #84914

Draft
bcoca wants to merge 1 commit into
ansible:develfrom
bcoca:warn_more_consistent
Draft

Fix warning consistency and internal clobbers#84914
bcoca wants to merge 1 commit into
ansible:develfrom
bcoca:warn_more_consistent

Conversation

@bcoca

@bcoca bcoca commented Apr 1, 2025

Copy link
Copy Markdown
Member
ISSUE TYPE
  • Bugfix Pull Request

@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 1, 2025
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Apr 3, 2025
@bcoca
bcoca force-pushed the warn_more_consistent branch from 7c6b05b to 92b767e Compare April 3, 2025 22:17
@ansible ansible deleted a comment from ansibot Apr 3, 2025
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 3, 2025
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Apr 4, 2025
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 4, 2025
@ansible ansible deleted a comment from ansibot Apr 4, 2025
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 4, 2025
@bcoca
bcoca marked this pull request as ready for review April 4, 2025 16:43
@bcoca

bcoca commented Apr 4, 2025

Copy link
Copy Markdown
Member Author

nvmd

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 7, 2025
@bcoca
bcoca force-pushed the warn_more_consistent branch from 3c43f37 to de4922e Compare April 8, 2025 14:45
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 8, 2025
@bcoca
bcoca requested review from mattclay and s-hertel April 8, 2025 19:52
@bcoca
bcoca force-pushed the warn_more_consistent branch from de4922e to 57fed1c Compare April 14, 2025 16:42
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 14, 2025
@ansible ansible deleted a comment from ansibot Apr 14, 2025
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Apr 15, 2025
@mattclay

Copy link
Copy Markdown
Member

@bcoca Can you resolve the CI issues on this PR? I'd like to wait until it's passing before I review it.

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 18, 2025
@bcoca
bcoca force-pushed the warn_more_consistent branch from 94c45af to 95cd4ae Compare April 21, 2025 19:08

@s-hertel s-hertel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change seems like a good improvement, will add warnings for 43 (in my latest count) internal variables. I think we should reduce the erroneous warnings too. Adding/removing warnings doesn't impact playbook behavior, so I think those changes specifically should be backported.

I tested name with various types of params (since you gave it as a reason why we can't just basically delete https://github.com/ansible/ansible/blob/v2.18.5/lib/ansible/vars/reserved.py#L38-L65), back to 2.2 before the original warning was added. I don't see a compelling reason to keep a warning for that.

playbook.yml
# Comment out the rest of the playbook for each test, as warnings are deduplicated. The comments are a summary of that process.
- hosts: localhost
  gather_facts: no
  roles:
    - name: role1  # no warning, of course - params never emit a warning, this has been consistent
      vars:
        role: warning from 2.3 (f4b6b72c5b009d6b9eb947382b38f3e337d264b7) to 2.15 (0b678d5036f64f95d50d73d3d27a523a2b264050), 2.17.8+/2.18.2+ (819e437d967e33f7331cbc28e7d18920f7fa5740 / 3e22739cc8e9542c070e561d5e8457df594c9669).

    - role: role1
      name: no warning
      vars:
        name: warning from 2.3 (f4b6b72c5b009d6b9eb947382b38f3e337d264b7) to 2.15 (0b678d5036f64f95d50d73d3d27a523a2b264050), 2.17.8+/2.18.2+ (819e437d967e33f7331cbc28e7d18920f7fa5740 / 3e22739cc8e9542c070e561d5e8457df594c9669).
  tasks:
    # Tested 2.3 through 2.15 since include does not exist in 2.16.
    - include:
        _raw_params: roles/role1/tasks/main.yml
        name: no warning
        vars:
          name: no warning

    # Tested 2.4 through devel, since it doesn't exist in 2.3.
    - import_role:
        name: role1
      vars:
        name: warning from 2.7 (27b4d7ed31b6688253fc4089b7a6b97f2d548167) to 2.15 (0b678d5036f64f95d50d73d3d27a523a2b264050), 2.17.8+/2.18.2+ (819e437d967e33f7331cbc28e7d18920f7fa5740 / 3e22739cc8e9542c070e561d5e8457df594c9669).

    # Tested 2.3 through devel since include_role does not exist in 2.2.
    - include_role:
        name: role1
      vars:
        name: warning from 2.17.8+/2.18.2+ (819e437d967e33f7331cbc28e7d18920f7fa5740 / 3e22739cc8e9542c070e561d5e8457df594c9669).
roles/role1/meta/main.yml
dependencies:
  - role: role2
    name: no warning
    vars:
       name: warning from 2.3 (f4b6b72c5b009d6b9eb947382b38f3e337d264b7) to 2.15 (0b678d5036f64f95d50d73d3d27a523a2b264050), 2.17.8+/2.18.2+ (819e437d967e33f7331cbc28e7d18920f7fa5740 / 3e22739cc8e9542c070e561d5e8457df594c9669).

name is fully defined by the user, ansible does not create it or utilize it magically anywhere. The warning makes it appear as though there is a problem when it works like normal variable precedence, nothing to warn over.

It's right twice...
environment
vars
and wrong 62 times - all variables ansible warns about, but does not create/utilize.
action
always
any_errors_fatal
args
async
async_val
become
become_exe
become_flags
become_method
become_user
block
changed_when
check_mode
collections
connection
debugger
delay
delegate_facts
delegate_to
diff
fact_path
failed_when
force_handlers
gather_facts
gather_subset
gather_timeout
handlers
hosts
ignore_errors
ignore_unreachable
local_action
loop
loop_control
loop_with
max_fail_percentage
module_defaults
name
no_log
notify
order
poll
port
post_tasks
pre_tasks
register
remote_user
rescue
retries
roles
run_once
serial
strategy
tags
tasks
throttle
timeout
until
vars_files
vars_prompt
when
with_

If we remove that block of code, then we'd just need to add environment and vars to INTERNAL_STATIC_VARS.

Comment thread lib/ansible/constants.py
Comment thread lib/ansible/constants.py
Comment thread lib/ansible/vars/manager.py
Comment thread changelogs/fragments/reserved_vars_fix.yml Outdated
@bcoca

bcoca commented Apr 22, 2025

Copy link
Copy Markdown
Member Author

I'll setup an example, the name you are using above is something we fixed long ago, it was just the thing that alerted us to the issue. Passing vars inline is not an issue, passing a variable inline that collides with role defs was. Though I suspect you are correct about it being needed anymore, we've restructured how all this worked and never had tests to check if the conflict was still there or resolved.

@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Apr 22, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 6, 2025
@bcoca
bcoca force-pushed the warn_more_consistent branch from 51c676d to 76d816f Compare May 15, 2025 14:02
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels May 15, 2025
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label May 26, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 26, 2025
@bcoca
bcoca force-pushed the warn_more_consistent branch from 76d816f to 70129fb Compare June 17, 2025 20:24
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jun 17, 2025
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Jun 18, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 2, 2025

@mattclay mattclay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bcoca Can you resolve the CI issues on this PR? I'd like to wait until it's passing before I review it.

@bcoca
bcoca marked this pull request as draft July 25, 2025 18:47
@bcoca

bcoca commented Jul 25, 2025

Copy link
Copy Markdown
Member Author

@mattclay i still have unanswered questions, specially when it comes to ansible_local and it's
'overwrittablity'

protect some internals from overwrite
updated test to use one internal from new source
added missing vars as per s-hertel
also handle loop vars

Co-authored-by: Sloane Hertel <19572925+s-hertel@users.noreply.github.com>
@bcoca
bcoca force-pushed the warn_more_consistent branch from 70129fb to 4320ec0 Compare July 25, 2025 20:30
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 25, 2025
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Jul 28, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 4, 2025
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Aug 26, 2025
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This issue/PR relates to a bug. ci_verified Changes made in this PR are causing tests to fail. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_pr This PR has not been pushed to for more than one year.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants