Skip to content

[pytest] Improve ansible_host #1006

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

Merged
lguohan merged 4 commits into from
Jul 28, 2019
Merged

[pytest] Improve ansible_host #1006

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b6a8016
[pytest] adding ability to use stdout callback plugin when running an…
Jul 12, 2019
e6bed55
[pytest ansible_host] Support ignore_errors options
Jul 12, 2019
484f182
[pytest ansible_host] Fix typo of callback name
Jul 15, 2019
6f3c9e0
[pytest ansible_host] rename 'ignore_errors' to 'module_ignore_errors'
Jul 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions tests/ansible_host.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
from ansible.plugins import callback_loader

def dump_ansible_results(results, stdout_callback='yamll'):
cb = callback_loader.get(stdout_callback)
return cb._dump_results(results) if cb else results

class ansible_host():

def __init__(self, ansible_adhoc, hostname, is_local = False):
Expand All @@ -15,8 +21,10 @@ def __getattr__(self, item):

def _run(self, *module_args, **complex_args):

ignore_errors = complex_args.pop('ignore_errors', False)
Copy link
Copy Markdown
Contributor

@stepanblyschak stepanblyschak Jul 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore_errors is not a module argument. I would prefer user code to catch exception

Copy link
Copy Markdown
Contributor Author

@dawnbeauty dawnbeauty Jul 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stepanblyschak
two reasons:

  1. ignore_errors is not a module argument, so it should be popped here instead of pass into module.
    OFC, it could be passed in as a task argument and handled in play_ds of pytest-ansible.module_dispatcher.v<ver>.ModuleDispatcherV<ver>._run, but it need to make more changes with pytest-ansible module.

  2. With non-zero code, user could catch exceptions, but could not get results if they need to do sth with it.
    e.g.

- shell: 'cmd return non-zero code'
  register: out
  failed_when: 'expected keywords' in out

With ignore_errors, we could easily translate it into pytest:

res  = host.shell('cmd', ignore_errors=True)
assert 'expected keywords' in res['stdout']

Copy link
Copy Markdown
Contributor

@stepanblyschak stepanblyschak Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with 2, maybe need to add AnsibleModuleException which will include 'res' with pretty printed 'res' by overriding repr?

Copy link
Copy Markdown
Contributor Author

@dawnbeauty dawnbeauty Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good advise. But, do you mean user extract results from AnsibleModuleException if need? Sometimes, we don't known about whether the cmd return non-zero code or not. For cover these, we might need to code like:

try:
   res = host.shell("cmd")
except AnsibleModuleException as exc:
   res = exc.results
assert 'some keywords' in res['stdout']

Even wrose, we might need to execute a sequence of 'non-zero' cmds. There are too many try/except blocks.

Maybe add AnsibleModuleException include pretty-printed 'res' and keep 'ignore_errors'?

Copy link
Copy Markdown
Contributor

@stepanblyschak stepanblyschak Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this can lead to many try/except. A shortcut function in ansible_host to return result even on AnsibleModuleException can be an option. e.g:

res = host.ignore_errors('shell', 'cmd')

Which is a bit ugly but does not limit modules about 'ignore_errors' key.
If we can guaranty that non of existing modules will use 'ignore_errors' argument I am Ok with your proposal.

Copy link
Copy Markdown
Contributor Author

@dawnbeauty dawnbeauty Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only found M 'cp_network' will use 'ignore_errors' in ansible-devel(master branch).
In ansible-2.0.0.1, non existing modules use it as argument.

3 alternatives:

  1. add shortcut function. host.ignore_errors('shell', 'cmd')
  2. add argument 'module_ignore_errors' (module could use ignore_errors if necessary ). host.shell('cmd', module_ignore_errors=True)
  3. (new) add attr(or function) to enable/disable ignore_errors. host.ignore_errors=True; host.shell('cmd'); host.ignore_errors=False

I would prefer 2>1>3.


res = self.module(*module_args, **complex_args)[self.hostname]
if res.is_failed:
raise Exception("run module {} failed, errmsg {}".format(self.module_name, res))
if res.is_failed and not ignore_errors:
raise Exception("run module {} failed, errmsg {}".format(self.module_name, dump_ansible_results(res)))

return res