Skip to content
This repository has been archived by the owner on Jan 1, 2023. It is now read-only.

Fix image building under Python 3 #10

Merged
merged 4 commits into from Oct 20, 2015
Merged

Fix image building under Python 3 #10

merged 4 commits into from Oct 20, 2015

Conversation

ghost
Copy link

@ghost ghost commented Oct 18, 2015

When building a dockermap-generated Dockerfile under Python 3, we would
raise an exception in DockerContext.add_dockerfile() at line
self.tarfile.addfile(tarinfo, dockerfile_obj) because bytes are
expected, not strings (which is what StringIO stores).

I've fixed this by replacing StringIO by BytesIO and encoding
strings in Dockerfile.write*().

Also, fixed a bytes/str-related crash in the JSON parsing of the
response stream.

Sorry about the lack of tests, I really don't know the library enough to
write them (I haven't managed to successfully use it yet).

Virgil Dupras added 4 commits October 17, 2015 20:50
When building a dockermap-generated Dockerfile under Python 3, we would
raise an exception in `DockerContext.add_dockerfile()` at line
`self.tarfile.addfile(tarinfo, dockerfile_obj)` because bytes are
expected, not strings (which is what `StringIO` stores).

I've fixed this by replacing `StringIO` by `BytesIO` and encoding
strings in `Dockerfile.write*()`.

Also, fixed a bytes/str-related crash in the JSON parsing of the
response stream.

Sorry about the lack of tests, I really don't know the library enough to
write them (I haven't managed to successfully use it yet).
We would crash during `json.dumps()` under Python 3 because of the
`encoding` argument. I've removed it because it already defaults to
`utf-8` under Python 2.
We would crash during `json.dumps()` under Python 3 because of the
`encoding` argument. I've removed it because it already defaults to
`utf-8` under Python 2.
We would crash if `response` was a string because it doesn't have a
`decode()` method.
@ghost
Copy link
Author

ghost commented Oct 19, 2015

Having continued to use the library, I've stumbled upon quite a few more Python 3 crashes.

Also, I've noticed that test_backward_resolution_order is flaky (fails intermittently) under Python 3, even in the current master branch.

@ghost
Copy link
Author

ghost commented Oct 19, 2015

Oh, by the way, since it's obvious that I'm the only python 3 user of this library, you might be interested in my use case, which can be found at https://github.com/hsoft/wordpress-template-docker

merll pushed a commit that referenced this pull request Oct 20, 2015
Fix image building under Python 3
@merll merll merged commit ed666ba into merll:master Oct 20, 2015
@merll
Copy link
Owner

merll commented Oct 20, 2015

That's great, thank you! Both libraries that I have used to set up production environments (Fabric and SaltStack) do not support Python 3 yet. So getting to look into a real use case helps a lot. I will get back to you as soon as I make some progress there.

@merll
Copy link
Owner

merll commented Oct 22, 2015

I made some more changes in relation to string encoding in Dockerfiles and verified it works both for Python 2 and 3. Additionally there were some truncated line endings during log output.

Although that was not related, I removed the custom log levels for avoiding compatibility issues with other libraries. They should not be necessary - using INFO should be fine. That makes the console output of your project easier to read as well.

The flaky test test_backward_resolution_order was checking on a condition that was neither guaranteed nor necessary. It was trying to establish an order between two container configurations that were not dependent and therefore have no defined order. This should work correctly now.

Thanks again for providing the PR, reporting further issues, and for the useful case!

@ghost
Copy link
Author

ghost commented Oct 22, 2015

Thanks. I like this library because it looks a lot like what I was trying to build myself. I hope to be able to contribute more to the project soon enough (notably on the API documentation side, which I think could be improved and I could help with that since I have a fresh look to it)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant