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

Track source of vars #66102

Merged
merged 1 commit into from
Jan 8, 2020
Merged

Track source of vars #66102

merged 1 commit into from
Jan 8, 2020

Conversation

agaffney
Copy link
Contributor

@agaffney agaffney commented Dec 28, 2019 •
edited
Loading

SUMMARY

Track the source of variables when calling get_vars() in VarsManager

This is useful for debugging where ansible is getting a particular variable and for doing things like throwing deprecation warnings when using variables from certain sources (such as top-level facts).

This implementation cheats a bit by tracking which precedence level (and which file source, if available) a var comes from as the varset is built in get_vars(), rather than tracking each var from its original source. While creating a variable proxy/wrapper class and using it for each var would be the "better" approach, it's not practical to do so for simple variable types such as bool and int without breaking a lot of things or requiring a lot of hacks, mostly due to the JSON encoder.

This PR is the spiritual successor for #41811

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

VarsManager

ADDITIONAL INFORMATION

N/A

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.10 This issue/PR affects Ansible v2.10 feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Dec 28, 2019
@agaffney agaffney force-pushed the vars_source branch 3 times, most recently from b9e6e6c to 1edb2d0 Compare December 28, 2019 04:24
@ansible ansible deleted a comment from ansibot Dec 28, 2019
@ansible ansible deleted a comment from ansibot Dec 28, 2019
@agaffney agaffney changed the title [WIP] Track source of vars Track source of vars Dec 28, 2019
@agaffney agaffney removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Dec 28, 2019
@ansibot ansibot added the core_review In order to be merged, this PR must follow the core review workflow. label Dec 28, 2019
@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 Jan 5, 2020
@ansibot ansibot removed 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 Jan 7, 2020
abadger
abadger reviewed Jan 7, 2020
View reviewed changes
lib/ansible/vars/manager.py Outdated Show resolved Hide resolved
abadger
abadger reviewed Jan 7, 2020
View reviewed changes
lib/ansible/vars/manager.py Outdated Show resolved Hide resolved
abadger
abadger reviewed Jan 7, 2020
View reviewed changes
lib/ansible/vars/manager.py Outdated Show resolved Hide resolved
@abadger
Copy link
Contributor

abadger commented Jan 7, 2020

Does this implementation only tag the toplevel vars with source? If so, you should add a comment in a few places (I don't see an obvious single place where the comment should live) that notes that is the case.

@agaffney
Copy link
Contributor Author

agaffney commented Jan 7, 2020

Yes, this only tracks top-level vars. I'm not even sure how you would track non-top-level vars to make it meaningful to add that comment.

@abadger
Copy link
Contributor

abadger commented Jan 7, 2020

But the fact that it is very hard to track non-top-level vars is not a reason not to comment that only toplevel vars are tracked.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jan 7, 2020
@agaffney agaffney force-pushed the vars_source branch 2 times, most recently from a3aad60 to 3153fed Compare January 7, 2020 22:32
abadger
abadger reviewed Jan 7, 2020
View reviewed changes
lib/ansible/vars/manager.py Outdated Show resolved Hide resolved
abadger
abadger reviewed Jan 7, 2020
View reviewed changes
lib/ansible/vars/manager.py Outdated Show resolved Hide resolved
@abadger
Copy link
Contributor

abadger commented Jan 7, 2020

Added a couple of suggestions for comments pointing to the primary documentation of how vars source tracking works.

Suggestion for the docstring of the new classmethod that might make it clearer to someone coming fresh to the code.

Looks good to me after the first two are merged. The docstring change is at your option.

@abadger
Copy link
Contributor

abadger commented Jan 7, 2020

rebuild_merge

@abadger
Copy link
Contributor

abadger commented Jan 8, 2020

Failing tests are unrelated to this PR. Merging.

@abadger abadger merged commit 45713aa into ansible:devel Jan 8, 2020
@agaffney agaffney deleted the vars_source branch January 8, 2020 01:37
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jan 9, 2020
@ansible ansible locked and limited conversation to collaborators Feb 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sivel sivel sivel left review comments

@abadger abadger abadger left review comments

Assignees
No one assigned
Labels
affects_2.10 This issue/PR affects Ansible v2.10 feature This issue/PR relates to a feature request. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@agaffney @abadger @sivel @bcoca @ansibot