Support new display options in ansible-core

[AlanCoding]

wtih https://github.com/ansible/ansible/pull/76166, these configuration options were introduced:

export ANSIBLE_CALLBACK_RESULT_FORMAT=yaml
export ANSIBLE_CALLBACK_FORMAT_PRETTY=1

These do not work in ansible-runner because the callback does not extend the doc fragment for this.

It is unclear if this can be made to work with cross-version compatibility.

[AlanCoding]

I confirmed that this will show the desired display change

diff --git a/ansible_runner/display_callback/callback/awx_display.py b/ansible_runner/display_callback/callback/awx_display.py
index 0388a9a..871d0ef 100644
--- a/ansible_runner/display_callback/callback/awx_display.py
+++ b/ansible_runner/display_callback/callback/awx_display.py
@@ -27,6 +27,7 @@ DOCUMENTATION = '''
     type: stdout
     extends_documentation_fragment:
       - default_callback
+      - result_format_callback
     requirements:
       - Set as stdout in config
 '''
diff --git a/demo/env/envvars b/demo/env/envvars
index 8d0b681..498bcda 100644
--- a/demo/env/envvars
+++ b/demo/env/envvars
@@ -1,2 +1,3 @@
 ---
 TESTVAR: aval
+ANSIBLE_CALLBACK_RESULT_FORMAT: yaml

Next, I checked out the ansible core commit fd4460c1e4d8e8f8d482b3c25aac922ad7065154 and re-ran. Got this error:

(env) [alancoding@alan-red-hat ansible-runner]$ ansible-runner run demo/ -p test.yml -v
[WARNING]: You are running the development version of Ansible. You should only
run Ansible from "devel" if you are modifying the Ansible engine, or trying out
features under development. This is a rapidly changing source of code and can
become unstable at any point.
No config file found; using defaults
ERROR! unknown doc_fragment(s) in file /home/alancoding/repos/ansible-runner/ansible_runner/display_callback/callback/awx_display.py: result_format_callback

As expected, listing a doc fragment that doesn't exist will lead to an error. So this leaves us in a bit of a bind trying to write our callback plugin for multiple versions.

[nitzmahone]

I can't remember how we handle duplicated entries on doc fragments, but IIRC (and I would hope that) the more local definition would silently "win". So probably the sanest thing here would be to either duplicate the missing entries (and also make sure core has tests that explicitly assert that behavior working the way I think it does ;) ) or drop the doc fragment entirely and duplicate all the necessary bits that runner's version of the plugin actually supports.

[AlanCoding]

^ yes, this is the direction I thought it would probably go in.

In other cases, we import the callback loader from Ansible and use that to get the parent class. Could the doc fragment (plugin?) do the same thing? Then if the callback loader returned the name we expected (result_format_callback) then we would like to return that unmodified. If it doesn't find that plugin, then we could just have a stub.

Not sure if that works, but it's worth a try.

[nitzmahone]

Unless I'm misunderstanding how you want to accomplish it, that would involve a fair bit of replumbing deep in core's config support for doc fragments, which isn't going to help provide something generic up at the actual plugin/doc level (and would certainly not be a good backport candidate, which doesn't help with older versions either).

Bigger picture: I've still never understood why runner subclasses default for this anyway (which is most definitely unsupported API), instead of just adding a second event-dumping callback (which is a 100% supported API) and filtering or capturing stdout/stderr as necessary for the "pretty" end of things from runner's subprocess launch of Ansible.

[AlanCoding]

My prior comment was imprecise without code. We do this:

https://github.com/ansible/ansible-runner/blob/88130033e6b4791952236a8b1eb6f7ac823649e3/ansible_runner/display_callback/callback/awx_display.py#L64

The argument becomes a string which is either "minimal" or "default" under normal circumstances. So we're calling ansible-core's code and it returns to us a class which we then extend.

Doc fragment plugins follow a pattern which is more rigid than other plugins, example:

https://github.com/ansible-collections/community.general/blob/main/plugins/doc_fragments/rackspace.py

But it's still python code. I'm questioning if it is viable to use the callback loader to conditionally load an ansible.builtin doc fragment and return that under certain conditions. If so, then we could do a single patch to runner here and close this because it could work for all versions correctly. Instead of callback_loader, we would use fragment_loader.

[bcoca]

simpler to just add your fragment to a local doc_fragments dir, it will be picked up automatically, if you want it at less precedence you can control that by adding it to the configured paths as the last item.

[AlanCoding]

^ that may allow a one-shot solution that's compatible with ansible-core both before and after that change.

As a general principle, I would like that ansible-runner avoid modifying any Ansible setting, because it winds up overwriting user's values. If it becomes important to support new features, then it might still be the least bad option.

[bcoca]

going forward 'ansible-config get' will allow you to modify incrementally ... but that wont be for a while (also looking to 'incremental add' directly feature)

export ANSIBLE_DOC_FRAGMENT_PLUGINS=$(ansible-config get 'DOC_FRAGMENT_PLUGIN_PATH'):/also/my/path

[nitzmahone]

Depending on how "clever" you want to be and what Python versions you need this patched version to support, another possibility might be reimplementing your plugin's DOCUMENTATION attr dynamically via a module-level __getattr__ (which IIRC has been supported since Python 3.7), and just have it pass through/merge with the value from the default plugin (rather than the static string it is now, which IIUC is the ultimate problem). Come to think of it, since all that stuff is happening at module scope anyway, you don't even need to it be dynamic at eval-time- just overwrite it with the value from the real plugin's module as soon as you've loaded the real plugin. That should work on every Python version.

If either of those approaches hit sanity test failures or breaks other things in the plugin loader, those are arguably bugs we should fix...

[nitzmahone]

and in the spirit of @AlanCoding's "imprecise without code" comment earlier :wink:, perhaps something like :

diff --git a/ansible_runner/display_callback/callback/awx_display.py b/ansible_runner/display_callback/callback/awx_display.py
index 0388a9a..f2e0447 100644
--- a/ansible_runner/display_callback/callback/awx_display.py
+++ b/ansible_runner/display_callback/callback/awx_display.py
@@ -62,6 +62,7 @@ else:
     default_stdout_callback = 'default'
 
 DefaultCallbackModule = callback_loader.get(default_stdout_callback).__class__
+DOCUMENTATION = sys.modules[DefaultCallbackModule.__module__].DOCUMENTATION
 
 CENSORED = "the output has been hidden due to the fact that 'no_log: true' was specified for this result"

(and if you want/need to adjust values in the YAML at runtime, wrap it in a basic mutator function to do that on the fly)

That leaves the static value there for humans looking at the code or any other sanity stuff that might look at it, but by the time core's plugin loader picks it up, it'll have exactly what the corresponding core version's plugin has.

[AlanCoding]

I love it!