-
Notifications
You must be signed in to change notification settings - Fork 366
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
Implement ansible's resolved action #995
Conversation
0faa706
to
9740494
Compare
98ea35a
to
e88acb9
Compare
ansible-core also depends on packaging so this seems like a reasonable dependency
487624e
to
47ee0e5
Compare
+1, not because I completely grok it all but because it doesn't use distulils :) |
@@ -46,6 +48,30 @@ def skipif_pre_ansible211(is_pre_ansible211): | |||
pytest.skip("Valid only on Ansible 2.11+") | |||
|
|||
|
|||
@pytest.fixture(scope="session") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not crazy about what the is_pre_ansible211
test does. It doesn't seem like it'll work in the general case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure how I feel about this implementation generally but it gets us there quick with whatever variant of Ansible is installed in the environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The is_pre_ansible211
fixture should work in the general case. The ansible package used in our tests will be ansible-core
for 2.11 or higher. 2.10 is ansible-base
, and 2.9 and earlier is ansible
.
@@ -1,6 +1,8 @@ | |||
import shutil | |||
|
|||
from pathlib import Path | |||
from packaging.version import Version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simaishi Can you check and see if this is available as an RPM from the standard repos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤞🏻 it's also used by ansible-core
so my working assumption was that we had solved it for the necessary dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ansible-core has it bundled, but we do have an access to the standalone rpm as well.
This looks most similar to the |
To give my comment greater specificity, diff --git a/ansible_runner/display_callback/callback/awx_display.py b/ansible_runner/display_callback/callback/awx_display.py
index aa29f39..0aee596 100644
--- a/ansible_runner/display_callback/callback/awx_display.py
+++ b/ansible_runner/display_callback/callback/awx_display.py
@@ -409,6 +409,7 @@ class CallbackModule(DefaultCallbackModule):
task=(task.name or task.action),
task_uuid=str(task._uuid),
task_action=task.action,
+ resolved_action=getattr(task, 'resolved_action', ''),
task_args='',
)
try:
@@ -554,7 +555,6 @@ class CallbackModule(DefaultCallbackModule):
name=task.get_name(),
is_conditional=is_conditional,
uuid=task_uuid,
- resolved_action=getattr(task, 'resolved_action', '')
)
with self.capture_event_data('playbook_on_task_start', **event_data):
super(CallbackModule, self).v2_playbook_on_task_start(task, is_conditional) this would seem more clear and still passes the test |
recheck |
Build succeeded.
|
@Shrews in the future, I want to make sure that we do integration testing with AWX before merging any change intended to be consumed by AWX. In this case, I did, and those were passing. |
@AlanCoding Ok. @shanemcd had asked me about merging this today, so I assumed that was already done. |
This supercedes #816
This also includes some black formatting changes.