sean-m-sullivan commented 6 years ago

Proposal: Make Yaml Callback Default in ansible

Author: Sean Sullivan @Wilk42 IRC: none

Date: 2018/04/20

Status: New
Proposal type: core
Targeted Release: future release
Estimated time to implement: less than a day

Motivation

The default ansible output can be hard to read at times. Since the yaml callback plugin is included with ansible, making it the default callback plugin would improve the ansible experience out of the box.

Problems

The main problem is that at times ansible error output is hard to read without using a text editor to break down the text into a more human readable way.

Solution proposal

Describe your suggested solution here.
in the default ansible config file add
```
# Use the YAML callback plugin.
stdout_callback = yaml
# Use the stdout_callback when running ad-hoc commands.
bin_ansible_callbacks = True
```
An example of its use can be found here: https://www.jeffgeerling.com/blog/2018/use-ansibles-yaml-callback-plugin-better-cli-experience

Dependencies (optional)

Yaml callback plugin

Testing (optional)

This is just a change to default behaviour of ansible

Documentation (optional)

No

caphrim007 commented 6 years ago

I also find the default output hard to read especially in cases of when all hell breaks loose in a module and I need to scan a python traceback line by line.

for example

fatal: [bigip1]: FAILED! => {
    "changed": false,
    "module_stderr": "Traceback (most recent call last):\n  File 
\"/tmp/ansible_vtwz7mje/ansible_module_bigip_security_log_profile.py\", line 883, in <module>\n  
  main()\n  File \"/tmp/ansible_vtwz7mje/ansible_module_bigip_security_log_profile.py\", line 874, in 
main\n    results = mm.exec_module()\n  File 
\"/tmp/ansible_vtwz7mje/ansible_module_bigip_security_log_profile.py\", line 614, in exec_module\n  
  changed = self.present()\n  File 
\"/tmp/ansible_vtwz7mje/ansible_module_bigip_security_log_profile.py\", line 636, in present\n    if 
self.exists():\n  File \"/tmp/ansible_vtwz7mje/ansible_module_bigip_security_log_profile.py\", line 642, 
in exists\n    result = self.client.api.tm.securigty.log.profiles.profile.exists(\n  File 
\"/usr/local/lib/python3.6/site-packages/f5/bigip/mixins.py\", line 102, in __getattr__\n    raise 
AttributeError(error_message)\nAttributeError: '<class 'f5.bigip.tm.Tm'>' object has no attribute 
'securigty'\n",
    "module_stdout": "",
    "msg": "MODULE FAILURE",
    "rc": 1
}

In my case, as a developer, providing -vvvv gives me a more readable error without using the YAML plugin.

The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_vtwz7mje/ansible_module_bigip_security_log_profile.py", line 883, in <module>
    main()
  File "/tmp/ansible_vtwz7mje/ansible_module_bigip_security_log_profile.py", line 874, in main
    results = mm.exec_module()
  File "/tmp/ansible_vtwz7mje/ansible_module_bigip_security_log_profile.py", line 614, in exec_module
    changed = self.present()
  File "/tmp/ansible_vtwz7mje/ansible_module_bigip_security_log_profile.py", line 636, in present
    if self.exists():
  File "/tmp/ansible_vtwz7mje/ansible_module_bigip_security_log_profile.py", line 642, in exists
    result = self.client.api.tm.securigty.log.profiles.profile.exists(
  File "/usr/local/lib/python3.6/site-packages/f5/bigip/mixins.py", line 102, in __getattr__
    raise AttributeError(error_message)
AttributeError: '<class 'f5.bigip.tm.Tm'>' object has no attribute 'securigty'

If I remove verbosity, I get this and only this (because verbosity gives you formatted output itself...in a way)

TASK [bigip_security_log_profile : Change network firewall publisher] ***********************************************************************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: 
AttributeError: '<class 'f5.bigip.tm.Tm'>' object has no attribute 'securitdy'
fatal: [bigip1]: FAILED! => {"changed": false, "module_stderr": "Traceback (most recent call last):\n  
File \"/tmp/ansible_dp0g2w26/ansible_module_bigip_security_log_profile.py\", line 883, in 
<module>\n    main()\n  File 
\"/tmp/ansible_dp0g2w26/ansible_module_bigip_security_log_profile.py\", line 874, in main\n  
  results = mm.exec_module()\n  File \"/tmp/ansible_dp0g2w26/ansible_module_bigip_security_log_profile.py\", line 614, in exec_module\n 
   changed = self.present()\n  File 
\"/tmp/ansible_dp0g2w26/ansible_module_bigip_security_log_profile.py\", line 636, in present\n    if 
self.exists():\n  File \"/tmp/ansible_dp0g2w26/ansible_module_bigip_security_log_profile.py\", line 
642, in exists\n    result = self.client.api.tm.securitdy.log.profiles.profile.exists(\n  File 
\"/usr/local/lib/python3.6/site-packages/f5/bigip/mixins.py\", line 102, in __getattr__\n    raise 
AttributeError(error_message)\nAttributeError: '<class 'f5.bigip.tm.Tm'>' object has no attribute 
'securitdy'\n", "module_stdout": "", "msg": "MODULE FAILURE", "rc": 1}

With the YAML plugin enabled, I get the same formatted output with -vvvv, however the unformatted part becomes

 [WARNING]: Failure using method (v2_runner_on_failed) in callback plugin (<ansible.plugins.callback.yaml.CallbackModule object at
0x7ffb7cb1ba58>): 'filter' object has no attribute 'expandtabs'

Callback Exception: 
  File "/usr/local/lib/python3.6/site-packages/ansible/executor/task_queue_manager.py", line 375, in
 send_callback
    method(*new_args, **kwargs)
   File "/usr/local/lib/python3.6/site-packages/ansible/plugins/callback/default.py", line 63, in 
v2_runner_on_failed
    self._display.display("fatal: [%s]: FAILED! => %s" % (result._host.get_name(), 
self._dump_results(result._result)), color=C.COLOR_ERROR)
   File "/usr/local/lib/python3.6/site-packages/ansible/plugins/callback/yaml.py", line 116, in 
_dump_results
    dumped += yaml.dump(abridged_result, width=1000, Dumper=AnsibleDumper, 
default_flow_style=False)
   File "/usr/local/lib/python3.6/site-packages/yaml/__init__.py", line 200, in dump
    return dump_all([data], stream, Dumper=Dumper, **kwds)
   File "/usr/local/lib/python3.6/site-packages/yaml/__init__.py", line 188, in dump_all
    dumper.represent(data)
   File "/usr/local/lib/python3.6/site-packages/yaml/representer.py", line 26, in represent
    node = self.represent_data(data)
   File "/usr/local/lib/python3.6/site-packages/yaml/representer.py", line 47, in represent_data
    node = self.yaml_representers[data_types[0]](self, data)
   File "/usr/local/lib/python3.6/site-packages/yaml/representer.py", line 205, in represent_dict
    return self.represent_mapping('tag:yaml.org,2002:map', data)
   File "/usr/local/lib/python3.6/site-packages/yaml/representer.py", line 116, in represent_mapping
    node_value = self.represent_data(item_value)
   File "/usr/local/lib/python3.6/site-packages/yaml/representer.py", line 47, in represent_data
    node = self.yaml_representers[data_types[0]](self, data)
   File "/usr/local/lib/python3.6/site-packages/yaml/representer.py", line 146, in represent_str
    return self.represent_scalar('tag:yaml.org,2002:str', data)
   File "/usr/local/lib/python3.6/site-packages/ansible/plugins/callback/yaml.py", line 52, in my_represent_scalar
    value = value.expandtabs()

Instead of the one-line output. Removing the verbosity, it results in

TASK [bigip_security_log_profile : Change network firewall publisher] ***********************************************************************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: 
AttributeError: '<class 'f5.bigip.tm.Tm'>' object has no attribute 'securitdy'
 [WARNING]: Failure using method (v2_runner_on_failed) in callback plugin 
(<ansible.plugins.callback.yaml.CallbackModule object at
0x7f6bb4449128>): 'filter' object has no attribute 'expandtabs'

Which...if I had to comment on it, is completely useless.

The point is that, by making the YAML plugin the default, you are hiding some very crucial error output for those who support modules.

So, my question is, is Jeff's example something that is relevant only to Jeff's example and not in the more broad context of output?

geerlingguy commented 6 years ago

Before this could happen, this difference between the default and YAML callback plugin needs to be reconciled: https://github.com/ansible/ansible/issues/39122

sivel commented 6 years ago

I honestly don't see this happening. For one, this would be a backwards incompatible change, as there are people relying on the current output.

geerlingguy commented 6 years ago

I honestly don't see this happening. For one, this would be a backwards incompatible change, as there are people relying on the current output.

That hasn’t stopped major changes in the past :) (e.g. import and include, static, etc, basically all the things that have been deprecated in the past 3 point releases and require rewrites before 2.8/2.9). And at least this change would be trivial to opt out of, it’s not like the default plugin would go away, it could just be renamed.

agaffney commented 6 years ago

Deprecating a feature and throwing a warning is different than changing a default setting without any warning.

jmcgill298 commented 6 years ago

@agaffney where do you see anyone suggesting to change a default setting without a warning? I only see a proposal to change the default behavior, but nothing on how/when the proposal should be implemented. Based on @geerlingguy comment, it would seem reasonable to throw a warning and make the change after the targeted release.

aoyawale commented 6 years ago

I would assume it will be done just like any changes we done in the past.

agaffney commented 6 years ago

I just assumed it would be done without a warning. However, I know that I would be really annoyed if a warning was emitted every time that I used ansible for 2-3 major versions unless I explicitly switched to the yaml callback plugin. I don't think that specific deprecation warnings can be disabled without turning them all off, which is not ideal.

It should also be noted that there are callback plugins that inherit from default (such as actionable), and work would need to be done to switch to inheriting from yaml or a decision made to NOT switch it.

jmcgill298 commented 6 years ago

So lets throw your assumption away, and say your actual concerns with finding a safe way to implement the change without overloading user experience with deprecation warnings, which is something that needs to be considered

geerlingguy commented 6 years ago

However, I know that I would be really annoyed if a warning was emitted every time that I used ansible for 2-3 major versions unless I explicitly switched to the yaml callback plugin.

Welcome to my personal hell of using Ansible through the 2.0 release cycle... almost every release (especially 2.2, 2.4, and 2.5) means I get about 100+ new GitHub issues and random PRs trying to update all my hundreds of roles and OSS playbooks to fix new deprecation warnings everyone sees, but these warnings pop on a new version and suggest something that only works on that new version.

So I have to let these issues sit until there's enough uptake with the new version that I'm comfortable making these breaking changes and forcing all my users to use the latest version (2.4 is/was the worst in this regard).

It's kind of a given with Ansible, though, and I'm used to it, so it doesn't bother me anymore. I'm used to seeing at least 2-4 deprecation warnings on every playbook run ever :)

dagwieers commented 6 years ago

@geerlingguy You basically did that to yourself on Galaxy ;-)

That said we have a proposal to not introduce something new + deprecate the old behaviour in the same release. So that when you get a warning, you know the previous release supports the new functionality as well. See https://github.com/ansible/proposals/issues/85

ahuffman commented 6 years ago

I like the new yaml callback, but one case I don't is on a multi-line command. It looks nice in my playbook code, but the yaml callback parser does weird things to it:

TASK [tower-rbac : Assign RBAC to Team | Project] ***************************************************************************************************************************
changed: [localhost] => changed=true 
  cmd:
  - tower-cli
  - role
  - grant
  - --team
  - JavaGuys
  - --type
  - use
  - --project
  - JavaGuys-build
  - --insecure

whereas the task is actually this:

- name: Assign RBAC to Team | Project
  command: 'tower-cli                                                  \
            role                                                       \
              grant                                                    \
                --team                 "{{ tower_rbac_team }}"         \
                --type                 "{{ tower_rbac_role }}"         \
                --project              "{{ tower_rbac_project }}"      \
                {% if not tower_verify_ssl %}
                --insecure
                {% endif %}
         '
  when: tower_rbac_project is defined

ansible / proposals

Make Yaml Callback Default in ansible #108

Proposal: Make Yaml Callback Default in ansible

Motivation

Problems

Solution proposal

Dependencies (optional)

Testing (optional)

Documentation (optional)