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.

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

[WIP] Show deprecation message when using top-level fact vars #41811

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

agaffney
Copy link
Contributor

@agaffney agaffney commented Jun 21, 2018

SUMMARY

Show deprecation message when using top-level fact vars

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

fact vars

ANSIBLE VERSION
ansible 2.7.0.dev0 (deprecate_top_level_facts 2f1059ead6) last updated 2018/06/21 14:41:45 (GMT -500)
ADDITIONAL INFORMATION

A few minor caveats: the deprecation warning doesn't show for something like debug: var=ansible_processor_count but does for debug: var=ansible_processor_count+0. This appears to be because ansible.template.AnsibleContext.resolve_or_missing() isn't being called for "simple" integer values like it is for string/dict/list values. This also does not support showing a warning for boolean values, because Python makes it almost impossible to create a bool-like object that works properly in all cases.

@agaffney
Copy link
Contributor Author

agaffney commented Jun 21, 2018

Test playbook:

- hosts: localhost
  vars:
    some_unsafe_var: !unsafe foo
    some_var_with_nested_fact:
      bar:
        - foo: '{{ ansible_hostname }}'
  tasks:
    - set_fact:
        some_set_fact_var: bar
    - name: (WARN) fact bare variable
      debug: var=ansible_hostname
    - name: (WARN) fact bare variable numeric
      debug: var=ansible_processor_count+0
    - name: (WARN) fact bare variable nested
      debug: var=some_var_with_nested_fact
    - name: fact via ansible_facts
      debug: var=ansible_facts.hostname
    - name: (WARN) fact via 'vars' lookup
      debug: msg="{{ lookup('vars', 'ansible_fqdn') }}"
    - name: non-fact bare variable
      debug: var=ansible_play_hosts
    - name: (WARN) fact via hostvars
      debug: var=hostvars[inventory_hostname].ansible_hostname
    - name: fact via hostvars with ansible_facts
      debug: var=hostvars[inventory_hostname].ansible_facts.hostname
    - name: non-fact unsafe var
      debug: var=some_unsafe_var
    - name: non-fact set_fact var
      debug: var=some_set_fact_var
    - name: (WARN) fact bare variable in template
      template:
        src: fact_vars.j2
        dest: /tmp/fact_vars.out

Test template:

just a test to see if we get a warning when we use a fact var such as {{ ansible_fqdn }} in a template

Output:

PLAY [localhost] ****************************************************************************************************************************************************************

TASK [Gathering Facts] **********************************************************************************************************************************************************
ok: [localhost]

TASK [set_fact] *****************************************************************************************************************************************************************
ok: [localhost]

TASK [(WARN) fact bare variable] ************************************************************************************************************************************************
[DEPRECATION WARNING]: Using top-level fact vars ('ansible_hostname') is deprecated. You should use the 'ansible_facts' dict instead. This feature will be removed in version 
2.9. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.
ok: [localhost] => {
    "ansible_hostname": "nuc2"
}

TASK [(WARN) fact bare variable numeric] ****************************************************************************************************************************************
[DEPRECATION WARNING]: Using top-level fact vars ('ansible_processor_count') is deprecated. You should use the 'ansible_facts' dict instead. This feature will be removed in 
version 2.9. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.
ok: [localhost] => {
    "ansible_processor_count+0": "1"
}

TASK [(WARN) fact bare variable nested] *****************************************************************************************************************************************
[DEPRECATION WARNING]: Using top-level fact vars ('ansible_hostname') is deprecated. You should use the 'ansible_facts' dict instead. This feature will be removed in version 
2.9. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.
ok: [localhost] => {
    "some_var_with_nested_fact": {
        "bar": [
            {
                "foo": "nuc2"
            }
        ]
    }
}

TASK [fact via ansible_facts] ***************************************************************************************************************************************************
ok: [localhost] => {
    "ansible_facts.hostname": "nuc2"
}

TASK [(WARN) fact via 'vars' lookup] ********************************************************************************************************************************************
[DEPRECATION WARNING]: Using top-level fact vars ('ansible_fqdn') is deprecated. You should use the 'ansible_facts' dict instead. This feature will be removed in version 2.9. 
Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.
ok: [localhost] => {
    "msg": "nuc2"
}

TASK [non-fact bare variable] ***************************************************************************************************************************************************
ok: [localhost] => {
    "ansible_play_hosts": [
        "localhost"
    ]
}

TASK [(WARN) fact via hostvars] *************************************************************************************************************************************************
[DEPRECATION WARNING]: Using top-level fact vars ('ansible_hostname') is deprecated. You should use the 'ansible_facts' dict instead. This feature will be removed in version 
2.9. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.
ok: [localhost] => {
    "hostvars[inventory_hostname].ansible_hostname": "nuc2"
}

TASK [fact via hostvars with ansible_facts] *************************************************************************************************************************************
ok: [localhost] => {
    "hostvars[inventory_hostname].ansible_facts.hostname": "nuc2"
}

TASK [non-fact unsafe var] ******************************************************************************************************************************************************
ok: [localhost] => {
    "some_unsafe_var": "foo"
}

TASK [non-fact set_fact var] ****************************************************************************************************************************************************
ok: [localhost] => {
    "some_set_fact_var": "bar"
}

TASK [(WARN) fact bare variable in template] ************************************************************************************************************************************
[DEPRECATION WARNING]: Using top-level fact vars ('ansible_fqdn') is deprecated. You should use the 'ansible_facts' dict instead. This feature will be removed in version 2.9. 
Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.
ok: [localhost]

PLAY RECAP **********************************************************************************************************************************************************************
localhost                  : ok=14   changed=0    unreachable=0    failed=0   

@ansibot ansibot added WIP affects_2.7 feature needs_triage support:core labels Jun 21, 2018
@samdoran samdoran removed the needs_triage label Jun 21, 2018
@agaffney agaffney force-pushed the deprecate_top_level_facts branch 4 times, most recently from 5c36101 to 66c634e Compare Jun 24, 2018
@ansibot ansibot added needs_ci stale_ci and removed needs_ci stale_ci labels Jun 24, 2018
@agaffney agaffney force-pushed the deprecate_top_level_facts branch 2 times, most recently from 72edf59 to d303be9 Compare Jun 24, 2018
@ansible ansible deleted a comment from ansibot Jun 24, 2018
@agaffney agaffney force-pushed the deprecate_top_level_facts branch 7 times, most recently from e864008 to e10f93c Compare Jun 25, 2018
@ansibot ansibot added needs_ci stale_ci and removed needs_ci stale_ci labels Jun 25, 2018
@nitzmahone
Copy link
Member

nitzmahone commented Jun 25, 2018

We don't necessarily need to do it now, but the way I'd originally played with this was updating AnsibleUnsafeX and UnsafeProxy as a more generic "tagged data" wrapper that we could use for various purposes later (eg tracking vars sourced from vault and preventing their display), and wiring it back into the JSON ser/deser stuff so that modules could apply tags as well. My original purpose for this was deprecating return values from modules and firing warnings when they were "touched" by J2 later, which of course implies that the wrappers can be created/preserved inside the modules.

Anyway, this doesn't preclude us from doing that later, so I'm +1 for some form of this.

@agaffney
Copy link
Contributor Author

agaffney commented Jun 29, 2018

A more generic "tagged data" wrapper does seem a logical extension of this PR. I already added a source param to AnsibleUnsafe. It's just a short hop from that to a generic data wrapper with both unsafe and source as params.

@agaffney
Copy link
Contributor Author

agaffney commented Jun 29, 2018

As a side note, I have a reconfirmed the inability of proxying bool values in Python, if simply because not all JSON implementations can have encoding functions overridden to properly format boolean values for use in JSON (true vs. True). This problem shows up in the set_fact integration test when it writes a jsonfile cache and tries to read it back in.

Edit: I was able to make this work through horrible hacks that involve looking at the calling function and trying to guess if it's part of the JSON encoder.

Edit 2: It seems the actual problem with overriding the JSON parser's encoding of certain types is that it will use its own handling for objects that subclass str/int/bool that gets triggered before it falls through to calling the default() function. Because the AnsibleUnsafe classes use the built-in type as a parent, the JSON encoder is only using its own built-in logic for those types. Trying to implement these wrapper classes without using the built-in type as a parent tends to break a lot of other random things.

@agaffney agaffney force-pushed the deprecate_top_level_facts branch from e10f93c to 60041bc Compare Jun 29, 2018
@agaffney agaffney force-pushed the deprecate_top_level_facts branch 3 times, most recently from 8c668ca to bec8e36 Compare Jun 30, 2018
@ansibot ansibot added the stale_ci label Jul 10, 2018
@ansibot ansibot added the needs_rebase label Jul 18, 2018
@bogdando
Copy link

bogdando commented Jul 24, 2018

Aside of given that change

    - name: (WARN) fact bare variable
      debug: var=ansible_hostname
    - name: fact via ansible_facts
      debug: var=ansible_facts.hostname

just as is would make all folks rewriting all ansible_foo things literally everywhere,
what would be the impact on custom inventory generators like:

- name: Add subnode-0 to inventory
  add_host:
    name: quux
    groups: quxes
    ansible_host: qux
    ansible_fqdn: baz
    ansible_user: bar
    ansible_private_key_file: foo

Will that qux host be accessible via ansible_facts.hostname and ansible_facts.host?

@agaffney
Copy link
Contributor Author

agaffney commented Jul 24, 2018

It should not change anything when using add_host, as those are not facts, and I don't think that add_host uses the ansible_facts mechanism to set those host vars. This will only affect vars that are set via the ansible_facts key in a module results dict, which applies to things like the setup module.

@ansibot ansibot added the support:community label Sep 21, 2018
@ansibot ansibot removed the support:community label Jun 25, 2019
@agaffney agaffney mentioned this pull request Dec 28, 2019
@ansibot ansibot added pre_azp and removed stale_ci labels Dec 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.7 feature needs_rebase pre_azp support:core WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants