-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
lxc_container: open files as text, fixes #30571 #30572
Conversation
@@ -722,7 +723,7 @@ def _config(self): | |||
|
|||
container_config_file = self.container.config_file_name | |||
with open(container_config_file, 'rb') as f: | |||
container_config = f.readlines() | |||
container_config = [to_text(line, errors='surrogate_or_strict') for line in f.readlines()] |
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.
Note: It's probably faster to write this like this:
container_config = to_text(f.read(), errors='surrogate_or_strict').splitlines()
There's overhead from calling the text conversion functions so calling them only once should be quicker.
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.
Also, I'm not sure if it matters but you could use to_native()
instead of to_text()
if container_config should be byte strings on python2 and text strings on python3. In a quick look at the following code I didn't see anything that would be a problem with text strings on python2 but I put that out there in case @cloudnull does see something.
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.
instead of using to_native, my first commit to just open the file with 'r' and write with 'w' might be cleaner. that way it will be bytes in py2 and text in py3. After reading the py3 docs assumed you want to always use to_text/to_bytes in those cases.
Correct. I definitely prefer that we open files in binary mode and use
to_bytes() when we write to them.
Reading from them when we're dealing with modules is trickier because we
let modules use a native string strategy. The modules can choose to try
dealing with strings as the str type on the python version they're running
as. This works well when there isn't much usage of the filesystem,
environment variables, over the wire protocols, or other things from
outside of python. In those cases,, having python strings match with bare
string literals can be very helpful for module developers. But when we do
start using files, filesystem paths, env vars, etc, even modules are forced
to start thinking about encodings.
Unfortunately, ```open()``` doesn't give much flexibility in terms of
encodings. So I still prefer to open the file in binary mode and use
to_text or to_native to convert the read in lines. But, depending on the
API being used, sometimes it makes sense to use to_text and other times it
makes sense to use to_native.
|
Merged to devel and cherry-picked to stable-2.4 for the 2.4.1 release. |
call to_text on full config file fixes issue ansible#30571
call to_text on full config file fixes issue ansible#30571
SUMMARY
fix python3 support
ISSUE TYPE
COMPONENT NAME
modules/cloud/lxc/lxc_container.py
ANSIBLE VERSION