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

docker_service python 3 compat #30696

Closed
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@dj4ngo
Contributor

dj4ngo commented Sep 21, 2017

SUMMARY

Fix Python 3 Compatibility docker_service

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

docker_service

ANSIBLE VERSION
ansible 2.5.0 (docker_services_py35_compat d295a6b1f2) last updated 2017/09/21 15:12:16 (GMT +200)
@ansibot

This comment has been minimized.

Contributor

ansibot commented Sep 21, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Sep 21, 2017

The test ansible-test sanity --test pep8 [?] failed with the following errors:

lib/ansible/modules/cloud/docker/docker_service.py:514:15: E201 whitespace after '['
lib/ansible/modules/cloud/docker/docker_service.py:514:34: E202 whitespace before ']'

click here for bot help

dj4ngo added some commits Sep 21, 2017

Clean code, fd.close not required since open as fd if used; remove ge…
…t_redirected_output from cleanup_redirection_tempfiles since output is not used
Solve issue with .encode('ascii') in python 3.5, replaced by to_text …
…method

Don't use --no-color option because replaced by --no-ansi to ensure compatibility

The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_y084ix0x/ansible_module_docker_service.py", line 1068, in <module>
    main()
  File "/tmp/ansible_y084ix0x/ansible_module_docker_service.py", line 1063, in main
    result = ContainerManager(client).exec_module()
  File "/tmp/ansible_y084ix0x/ansible_module_docker_service.py", line 657, in exec_module
    result = self.cmd_up()
  File "/tmp/ansible_y084ix0x/ansible_module_docker_service.py", line 753, in cmd_up
    cleanup_redirection_tempfiles(out_redir_name, err_redir_name)
  File "/tmp/ansible_y084ix0x/ansible_module_docker_service.py", line 515, in cleanup_redirection_tempfiles
    get_redirected_output(err_name)
  File "/tmp/ansible_y084ix0x/ansible_module_docker_service.py", line 524, in get_redirected_output
    new_line = re.sub(r'\x1b\[.+m', '', line.encode('ascii'))
  File "/usr/lib/python3.5/re.py", line 182, in sub
    return _compile(pattern, flags).sub(repl, string, count)
TypeError: cannot use a string pattern on a bytes-like object

@dj4ngo dj4ngo force-pushed the dj4ngo:docker_services_py35_compat branch to 177c609 Sep 21, 2017

@ansibot ansibot removed the ci_verified label Sep 21, 2017

@mscherer mscherer removed the needs_triage label Sep 21, 2017

@mscherer

This comment has been minimized.

Contributor

mscherer commented Sep 21, 2017

shipit

@dj4ngo

This comment has been minimized.

Contributor

dj4ngo commented Sep 21, 2017

@jctanner @kassiansun (as randomly selected maintainer of cloud/docker namespace) could you please review this pull request ? At least two shipit comments (from maintainers or namespace maintainers or core team members) are required in order to automerge this pull request.

@mscherer

This comment has been minimized.

Contributor

mscherer commented Sep 21, 2017

linked to #30672

@dj4ngo

This comment has been minimized.

Contributor

dj4ngo commented Sep 21, 2017

related to issue #30354

@kassiansun

This comment has been minimized.

Contributor

kassiansun commented Sep 22, 2017

@abadger This MR contains further cleanups, should we merge this instead of #30672?

def get_redirected_output(path_name):
output = []
with open(path_name, 'r') as fd:
for line in fd:
# strip terminal format/color chars
new_line = re.sub(r'\x1b\[.+m', '', line.encode('ascii'))
new_line = re.sub(r'\x1b\[.+m', '', to_text(line))

This comment has been minimized.

@abadger

abadger Sep 22, 2017

Member

This portion is wrong. On python3 it doesn't do anything (because line is already text). On python2 it makes a byte string pattern compare to a text string. If there's no nonascii characters I think that will work okay but it will definitely fail to do the right thing if nonascii does creep into the data.

@abadger

This comment has been minimized.

Member

abadger commented Sep 22, 2017

The python3 compatibility here is not correct. The other changes look good but I'm not a docker user or maintainer so I've been trying not to touch it too much.

@kassiansun

This comment has been minimized.

Contributor

kassiansun commented Sep 22, 2017

@abadger So for line in f returns bytes string on python 2.x? Then line needs to be converted with to_text as re.sub don't accept bytes string as argument on 2.7.x (higher versions) and 3.x (lower versions).

@abadger

This comment has been minimized.

Member

abadger commented Sep 22, 2017

That's incorrect. Both python2 and python3 accept both byte strings and text strings as arguments. What you shouldn't do is mix them together.

$ python2          *[python3-port-try2]  (19:41:18)
Python 2.7.13 (default, May 10 2017, 20:04:28) 
[GCC 6.3.1 20161221 (Red Hat 6.3.1-1)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import re
>>> re.sub(b'test', b'', b'this is a test case')
'this is a  case'

$ python3          *[python3-port-try2]  (19:41:57)
Python 3.5.4 (default, Aug 23 2017, 18:32:05) 
[GCC 6.4.1 20170727 (Red Hat 6.4.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import re
>>> re.sub(b'test', b'', b'this is a test case')
b'this is a  case'
output.append(new_line)
fd.close()

This comment has been minimized.

@abadger

abadger Sep 22, 2017

Member

Okay, I just tested this and it looks like fd.close() does not cause an error. It's useless to have it there but not strictly necessary to remove it.

@kassiansun

This comment has been minimized.

Contributor

kassiansun commented Sep 22, 2017

This is not the case with the source code:

Python 3.4.3 (default, Sep 14 2016, 12:36:27)
>>> re.sub(r'\x1b\[.+m', '', b'this is a test case')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.4/re.py", line 179, in sub
    return _compile(pattern, flags).sub(repl, string, count)
TypeError: can't use a string pattern on a bytes-like object
@abadger

This comment has been minimized.

Member

abadger commented Sep 22, 2017

@kassiansun You are doing exactly what I told you you can't do. You are mixing text strings r'\x1b\[.+m' and '' with a byte string: b'this is a test case'.

@kassiansun

This comment has been minimized.

Contributor

kassiansun commented Sep 22, 2017

But this is the case with source code, we need to make sure the third one is text string

def get_redirected_output(path_name):
output = []
with open(path_name, 'r') as fd:
for line in fd:
# strip terminal format/color chars
new_line = re.sub(r'\x1b\[.+m', '', line.encode('ascii'))
new_line = re.sub(r'\x1b\[.+m', '', to_text(line))

This comment has been minimized.

@kassiansun

kassiansun Sep 22, 2017

Contributor

There's no need to do to_text here, re.sub takes str as argument, bytes or text

This comment has been minimized.

@kassiansun

kassiansun Sep 22, 2017

Contributor

@abadger This behavior of re.sub is not consistent among python 3.x... But I think for line in f always return line as text str?

This comment has been minimized.

@abadger

abadger Sep 22, 2017

Member

@kassiansun python-3.x's re.sub has not changed. The basic rules of using it are even the same in python-2.x but python-2.x sometimes allows you to cheat (when the data you are operating on is ascii-only).

You also are incorrect about the type of line in for line in f. That depends on what f returns on iteration. In this case, f is a file which is opened in the default read mode. On Python-2.x, iterating f returns byte strings. On python3, it returns text strings. If desired, you can force it to return byte strings on both python2 and python3 by opening the file in binary mode ('rb') but there is no way to force it to return text strings on both python2 and python3.

This comment has been minimized.

@kassiansun

kassiansun Sep 22, 2017

Contributor

Thanks, that's the point I want to get. Your code is consistent among 2.x and 3.x, because for line in f returns line consistent with r''.

@abadger

This comment has been minimized.

Member

abadger commented Sep 22, 2017

@dj4ngo I've rebased and merged your cleanup commit on top of my fix for the python3 compat problem. The commit is here: 901bc2c

@kassiansun No. We need to make sure that all three of the strings are the same type (either all three bytes or all three text). My change, now merged does that. The change here creates a mismatch on Python2.

Additionally, this change would have changed the type of the string returned from get_redirected_output from a byte string to a text string on Python2. That could cause tracebacks in the calling code if it wasn't careful of how it combined the byte string with the

@abadger abadger closed this Sep 22, 2017

@ansibot ansibot added bug and removed bugfix_pull_request labels Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment