Skip to content
This repository has been archived by the owner on Nov 10, 2017. It is now read-only.

Preparations for merging with shub #21

Merged
merged 8 commits into from
Nov 23, 2016
Merged

Preparations for merging with shub #21

merged 8 commits into from
Nov 23, 2016

Conversation

vshlapakov
Copy link
Contributor

@vshlapakov vshlapakov commented Nov 22, 2016

There's an intention to merge shub and shub-image, but before it we should improve shub-image a bit to make the merge simpler and cleaner, this PR does the following things:

  • adds Python 3 support for shub-image
  • replaces ruamel dependency with PyYAML
  • moves docker-py to extras
  • updates other deps to make it work with latest shub tool
  • fixes tests (I also rewrote tests for test.py with pytest to get rid of mocking __builtins__).

Please, review.

@vshlapakov vshlapakov self-assigned this Nov 22, 2016
@vshlapakov vshlapakov changed the title [WIP] Preparations for merging with shub package [WIP] Preparations for merging with shub Nov 22, 2016
@codecov-io
Copy link

codecov-io commented Nov 22, 2016

Current coverage is 93.58% (diff: 94.44%)

No coverage report found for master at 9900b66.

Powered by Codecov. Last update 9900b66...08444e4

@@ -48,8 +47,7 @@ def build_cmd(target, version):
raise shub_exceptions.BadParameterException(
'Dockerfile is not found, please use shub-image init cmd')
is_built = False
for line in client.build(path=project_dir, tag=image_name):
data = json.loads(line)
for data in client.build(path=project_dir, tag=image_name, decode=True):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

decode param was added to build command in docker-py 1.3.0, to push/pull in 1.8.0, but works fine only since 1.10.0 version due to the bug; it explains picking docker-py>=1.10.0 version in extras.

line = line.strip()
if line:
yield line
else:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it doesn't make sense to try to split/strip a line and yield it again if the line is not string?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can actually drop this workaround since issues was fixed in docker-py 1.10 docker/docker-py#1059 (comment)

@vshlapakov vshlapakov assigned chekunkov and unassigned vshlapakov Nov 22, 2016
@vshlapakov vshlapakov changed the title [WIP] Preparations for merging with shub Preparations for merging with shub Nov 22, 2016
Copy link
Contributor

@chekunkov chekunkov left a comment

Choose a reason for hiding this comment

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

almost LGTM, check comments

],
extras_require={'docker-py': ['docker-py>=1.10.0']},
Copy link
Contributor

Choose a reason for hiding this comment

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

why extras, it's not an optional dependency for shub-image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I was beat out of reason when seeing a check for docker-py here.
We don't need this check then, do we? Dropped.

line = line.strip()
if line:
yield line
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can actually drop this workaround since issues was fixed in docker-py 1.10 docker/docker-py#1059 (comment)

@vshlapakov vshlapakov merged commit d479461 into master Nov 23, 2016
@vshlapakov vshlapakov deleted the shub-merge branch November 23, 2016 16:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants