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.

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

Add assertions testing the Docker v2 publish results. #139

Closed
wants to merge 1 commit into from

Conversation

bowlofeggs
Copy link

This commit adds several assertions to an existing sync/publish
CLI test case that ensure that the result of the pulp_docker v2
publish has the expected data that the Docker client will need.

@bowlofeggs
Copy link
Author

These tests are for https://pulp.plan.io/issues/1051.

self.assertTrue(
all([l.keys() == ['blobSum'] for l in manifest['fsLayers']]))
self.assertTrue(
all([isinstance(l['blobSum'], (str, unicode))
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails on Python 3.

>>> isinstance('foo', (str, unicode))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'unicode' is not defined

How about this?

isinstance('foo', (type(u''), type(b'')))
True

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I noticed that on Friday but it was 5 PM so I figured I'd save it for Monday. The type solution seems good. I was considering using the six library to fix this. Do you prefer your technique? It does avoid adding a dependency so that's good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Six would work well, and I certainly respect it as a lib. But it does seem excessive to add six as a dependency when there's so few use cases for it. That's the same reason I haven't added a random data generation lib - uuid4() and a few random.randint() calls have worked well enough.

Yeah I noticed that on Friday but it was 5 PM so I figured I'd save it for Monday.

👍

Copy link
Author

Choose a reason for hiding this comment

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

@Ichimonji10 I have made the change you suggested, and the tests now pass.

This commit adds several assertions to an existing sync/publish
CLI test case that ensure that the result of the pulp_docker v2
publish has the expected data that the Docker client will need.
@Ichimonji10
Copy link
Contributor

Merged as 423cb9a, with follow-up commits of 06d96a9 and 44b802a. Thank you, @rbarlow!

@Ichimonji10 Ichimonji10 closed this Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants