Skip to content
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

Jump to bottom

try/except xmltodict import, misc cleanups #16287

Merged
merged 1 commit into from
Aug 23, 2016
+14 −12
Merged

try/except xmltodict import, misc cleanups #16287

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 14 additions & 12 deletions lib/ansible/plugins/connection/winrm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,23 @@
import shlex
import traceback
import json
import xmltodict

from ansible.compat.six.moves.urllib.parse import urlunsplit

from ansible.compat.six.moves.urllib.parse import urlunsplit
from ansible.errors import AnsibleError, AnsibleConnectionFailure

try:
import winrm
from winrm import Response
from winrm.protocol import Protocol
except ImportError:
raise AnsibleError("winrm is not installed")

try:
import xmltodict
except ImportError:
raise AnsibleError("xmltodict is not installed")

Copy link
Contributor

Choose a reason for hiding this comment

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

Misleading error message? Should it say "xmltodict is not installed. Needed to use winrm connection type"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doh, indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I think about it, I definitely cargo cult'ed that import error idiom from the existing code. It doesn't seem to be common in the code base.

There are a couple of places with similar requirements (try to import something absolutely required, but for an optional plugin...). Is there a preferred way to handle those cases?

The try/except and AnsibleError seems reasonable (at least with the correct error message ;->). But there are a few approaches in the code:

plugins/cache/redis.py also raises an AnsibleError
plugins/cache/memcached.py prints() an error and calls sys.exit() (eek!)

[module_utils/basic.py does variations of sys.exit, but that's a special case...]

Copy link
Member

Choose a reason for hiding this comment

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

Since we don't actually need those present and functional to build docs, could just set HAS_PYWINRM/HAS_XMLTODICT and check them in CP init instead of raising immediately (though I suspect we may run into similar dependency issues in other plugins)

HAVE_KERBEROS = False
try:
import kerberos
Expand All @@ -49,15 +54,13 @@
from ansible.utils.hashing import secure_hash
from ansible.utils.path import makedirs_safe
from ansible.utils.unicode import to_bytes, to_unicode, to_str
from ansible.utils.vars import combine_vars

try:
from __main__ import display
except ImportError:
from ansible.utils.display import Display
display = Display()


class Connection(ConnectionBase):
'''WinRM connections over HTTP/HTTPS.'''

Expand Down Expand Up @@ -148,7 +151,7 @@ def _winrm_connect(self):

# open the shell from connect so we know we're able to talk to the server
if not self.shell_id:
self.shell_id = protocol.open_shell(codepage=65001) # UTF-8
self.shell_id = protocol.open_shell(codepage=65001) # UTF-8
display.vvvvv('WINRM OPEN SHELL: %s' % self.shell_id, host=self._winrm_host)

return protocol
Expand Down Expand Up @@ -182,7 +185,7 @@ def _winrm_send_input(self, protocol, shell_id, command_id, stdin, eof=False):
stream['#text'] = base64.b64encode(to_bytes(stdin))
if eof:
stream['@End'] = 'true'
rs = protocol.send_message(xmltodict.unparse(rq))
protocol.send_message(xmltodict.unparse(rq))

def _winrm_exec(self, command, args=(), from_exec=False, stdin_iterator=None):
if not self.protocol:
Expand All @@ -195,7 +198,7 @@ def _winrm_exec(self, command, args=(), from_exec=False, stdin_iterator=None):
command_id = None
try:
stdin_push_failed = False
command_id = self.protocol.run_command(self.shell_id, to_bytes(command), map(to_bytes, args), console_mode_stdin=(stdin_iterator == None))
command_id = self.protocol.run_command(self.shell_id, to_bytes(command), map(to_bytes, args), console_mode_stdin=(stdin_iterator is None))

# TODO: try/except around this, so we can get/return the command result on a broken pipe or other failure (probably more useful than the 500 that comes from this)
try:
Expand All @@ -220,10 +223,10 @@ def _winrm_exec(self, command, args=(), from_exec=False, stdin_iterator=None):
display.vvvvv('WINRM RESULT %r' % to_unicode(response), host=self._winrm_host)
else:
display.vvvvvv('WINRM RESULT %r' % to_unicode(response), host=self._winrm_host)

display.vvvvvv('WINRM STDOUT %s' % to_unicode(response.std_out), host=self._winrm_host)
display.vvvvvv('WINRM STDERR %s' % to_unicode(response.std_err), host=self._winrm_host)


if stdin_push_failed:
raise AnsibleError('winrm send_input failed; \nstdout: %s\nstderr %s' % (response.std_out, response.std_err))

Expand All @@ -239,7 +242,7 @@ def _connect(self):
self._connected = True
return self

def _reset(self): # used by win_reboot (and any other action that might need to bounce the state)
def _reset(self): # used by win_reboot (and any other action that might need to bounce the state)
self.protocol = None
self.shell_id = None
self._connect()
Expand Down Expand Up @@ -308,7 +311,7 @@ def _put_file_stdin_iterator(self, in_path, out_path, buffer_size=250000):
# cough up the data, as well as an indicator if this is the last chunk so winrm_send knows to set the End signal
yield b64_data, (in_file.tell() == in_size)

if offset == 0: # empty file, return an empty buffer + eof to close it
if offset == 0: # empty file, return an empty buffer + eof to close it
yield "", True

def put_file(self, in_path, out_path):
Expand Down Expand Up @@ -367,13 +370,12 @@ def put_file(self, in_path, out_path):
if not remote_sha1 == local_sha1:
raise AnsibleError("Remote sha1 hash {0} does not match local hash {1}".format(to_str(remote_sha1), to_str(local_sha1)))


def fetch_file(self, in_path, out_path):
super(Connection, self).fetch_file(in_path, out_path)
in_path = self._shell._unquote(in_path)
out_path = out_path.replace('\\', '/')
display.vvv('FETCH "%s" TO "%s"' % (in_path, out_path), host=self._winrm_host)
buffer_size = 2**19 # 0.5MB chunks
buffer_size = 2**19 # 0.5MB chunks
makedirs_safe(os.path.dirname(out_path))
out_file = None
try:
Expand Down