Enable check_mode in command module

[jradtilbrook]

SUMMARY

As per #15828. This adds support for check_mode where possible and leaves behaviour as is where not. This only works if supplying creates or removes since it needs something to base the heuristic off. If none are supplied it will just skip as usual.

Fixes #15828.

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

lib/ansible/modules/commands/command.py

ANSIBLE VERSION

ansible 2.5.2
  config file = None
  configured module search path = [u'/Users/jradtilbrook/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/Cellar/ansible/2.5.2/libexec/lib/python2.7/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.15 (default, May  1 2018, 16:44:08) [GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.1)]

ADDITIONAL INFORMATION

I wasn't too sure about the messages to use. Let me know if you want them changed to something better.

[maxamillion]

If this change is going to land, I think it should have an explicit explanation as to what check_mode means within the context of the module in the module's documentation.

[jradtilbrook]

Absolutely. I didn't want to spend too much time if it wasn't going to get accepted, but if you think it will I'm happy to add better docs. It looks like its built from the comments on the module so that's all I would need to do.

Are you okay with the current implementation? I'd prefer to lock that down before doing the documentation.

[nitzmahone]

I've actually done something in the past with custom versions of command/shell that had a check_mode_command arg that would run a different command under check mode (so as a playbook author you could do something different and guaranteed "not to change" things without a second task). I keep thinking we should explore that as an option as well, but this change wouldn't preclude that from happening in the future... +1 from me...

[abadger]

There's no conceptual problem with this. However, there are some implementation details to change.

• Right now, command returns changed=True by default. We need that to be the case in check mode as well.
• We don't use skipped for this type of thing. Returning changed=True rather than skipped=True is the right thing to do.
• It's probably simpler coding style to just put rc, out, err = module.run_command(args, executable=executable, use_unsafe_shell=shell, encoding=None, data=stdin) inside of a condition on module.check_mode, setting rc=0 and set out and err to some marker values (b'Command not run in check mode' perhaps?) when we're in check_mode.

[abadger]

Hold on, I was wrong about changed versus skipped. Talking it over with the other committers.

[jimi-c]

This will need changes to the documentation (as well as for shell.py, which has its own document stub) noting that the behavior under check mode has been slightly modified. This should also be noted in the porting guide for 2.6.

[jradtilbrook]

This is getting a bit stale now. I was waiting for @abadger to confirm after their comments before making any other changes. Should I continue to wait or progress with the changes requested by @jimi-c?

[samdoran]

I think the current changed or skipped reporting in this PR is accurate:

• If creates/deletes and in check mode, report changed True/False
• Otherwise skip in check mode

It would be nice to have an option to run another command in check mode, but that overcomplicates this change. That could be addressed in a future PR.

[abadger]

I agree with @samdoran You can ignore my skipped vs changed request. Do take a look at my third bullet point (about reducing the number of conditionals by simplifying the code), though

[jradtilbrook]

I've looked into your third point but can't come up with anything that is much better than what there is now. Either way, the conditionals for creates/removes and the glob checking needs to be done so it stays about as messy as it is now.
Perhaps you could elaborate further?

[abadger]

Instead of the checks inside of the removes and creates conditions above, conditionalize running this command on check_mode like this:

startd = datetime.datetime.now()

if not module.check_mode:
    rc, out, err = module.run_command(args, executable=executable, use_unsafe_shell=shell, encoding=None, data=stdin)
elif removes or creates:
    rc = 0
    stdout = stderr = b'Command would have run if not in check mode'
else:
    module.exit_json(msg="skipped, running in check mode", skipped=True)

[jradtilbrook]

Thank you, that is clearer but I don't think this would be what we want.

This would not let the user know if it would change or not, ie. it doesn't really add anything to what it currently does.

[jradtilbrook]

I'll carry on the conversation here, closer to the code it references ...

I'm assuming that your suggestion means that lines 218-258 (in my current code) would be removed, otherwise the middle elif removes or creates: will never be true because its handled above. In that case, my current code differs in that it will actually check whether the file exists and will report a changed value accordingly, whereas your suggestion will return a blanket maybe without actually checking.

I feel we might be getting wires crossed here so I'll reiterate my intentions just in case - I would like for this module to be able to handle check mode and use the provided creates/removes to return something other than the skipped it currently does.

Let me know if you were actually meaning for lines 218-258 to remain. If so, this still wouldn't have reduced the number of conditionals. Also let me know if I'm not clear enough about what I'd like the module to do. Sorry for any confusion 😄

[jradtilbrook]

Hmm I realised you were asking for an example (plus, I messed up the explanation anyway):

I use the module setting creates to some file it is supposed to create, I run the playbook in check mode.
In your suggestion it reports as changed but it actually might not have been.
In my current implementation it will report as changed if it doesn't exist and not if it does exist.

[abadger]

You are correct that I imagine the changes in 218-258 going away. However, the existing code should still run. So if you pass in creates and the file exists, line 222 should be true and the module will exit using the normal code path saying that it would not make a change.

If the file does not exist, we will progress to the conditional I propose and set stdout to say that the command would have run if not in check mode. Then the module will exit normally with changed=true. Am I wrong?

[jradtilbrook]

Ahh yes I see now and it all makes sense!! I'll update the code now but the docs update for the shell module I'll have to do another time (hopefully tomorrow). Sorry to keep bugging you

[abadger]

Added a comment on the code that shows what I meant.

[abadger]

I think it does.... Can you take it the code pay where it doesn't?

…

On Fri, Jul 13, 2018, 12:01 AM Jarryd Tilbrook ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/ansible/modules/commands/command.py <#40428 (comment)>: > if warn: check_command(module, args) + # skip if in check mode so no changes are made + if module.check_mode: + module.exit_json(msg="skipped, running in check mode", skipped=True) + startd = datetime.datetime.now() rc, out, err = module.run_command(args, executable=executable, use_unsafe_shell=shell, encoding=None, data=stdin) Thank you, that is clearer but I don't think this would be what we want. This would not let the user know if it *would* change or not, ie. it doesn't really add anything to what it currently does. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#40428 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMxWmvePLx_odGuTPQmKIM3mnMELNS4ks5uGEXMgaJpZM4UFgTc> .

[abadger]

Sorry, autocorrect on my phone really mangled that... Can you trace the code path for me where it outputs something differently than your version?

[jradtilbrook]

@jimi-c @maxamillion Is there any specific wording or place I should mention the changed behaviour of this module under check mode?
I'm thinking it fits under either the description or the notes section, and something like: The behavior under check mode has changed. If creates or removes is specified the module will perform checks to determine if a change will be made. If those are not provided it will be skipped

[maxamillion]

Nothing specific that comes to mind, but I'd want to make sure that anywhere in the docs we introduce the concept of check_mode there is a note about this. Also the module docs for the shell and command modules should reflect the change. @abadger @jimi-c thoughts?

[abadger]

* Notes section in the command and shell docs. * porting guide entry https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/porting_guides/porting_guide_2.7.rst * changelog entry ( You can look at the existing changelog fragments for how to do this. It's a minor feature so this might be a suitable example: https://github.com/ansible/ansible/blob/devel/changelogs/fragments/puppet_debugging_options.yaml _

…

On Tue, Jul 17, 2018 at 7:03 AM, Adam Miller ***@***.***> wrote: Nothing specific that comes to mind, but I'd want to make sure that anywhere in the docs we introduce the concept of check_mode there is a note about this. Also the module docs for the shell and command modules should reflect the change. @abadger <https://github.com/abadger> @jimi-c <https://github.com/jimi-c> thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#40428 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMxWkjAnYwccrAGRLr4PzDtSnTd3OYRks5uHe6ugaJpZM4UFgTc> .

[abadger]

@jradtilbrook Thanks for your hard work and patience getting this finalized! Merged to devel for the 2.7.0 release.

[jradtilbrook]

No worries, thanks for your help and sorry about all the back-and-forth from me trying to clarify! 😀