Skip to content

Conversation

posita
Copy link
Contributor

@posita posita commented May 17, 2015

Fix #602. Raise ValueError on empty argument to inspect_{container,image}() methods.

@posita posita changed the title Fix #602. Raise ValueError on empty argument to inspect_{container,image}() methods. READY FOR REVIEW - Fix #602. Raise ValueError on empty argument to inspect_{container,image}() methods. May 17, 2015
docker/client.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This if isinstance ... block wasn't part of the original issue, but is included to provide consistency with the inspect_container() method.

@shin-
Copy link
Contributor

shin- commented May 17, 2015

This would be better done in the check_resource decorator.

@posita
Copy link
Contributor Author

posita commented May 18, 2015

@shin, I've moved the check to @check_resource as you suggest (and squashed commits). Tests pass. Are you sure this won't have detrimental side effects elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't required to fix #602, but the change is make this method consistent with inspect_container.

@shin, incidentally, this also looks like a good candidate for inclusion in @check_resource. Similar checks are littered throughout client.py (but still missing from some methods). See my raise-on-inspect-empty-string-602-with-id-resolution-in-check-resource branch. (Tests pass.)

@posita
Copy link
Contributor Author

posita commented May 20, 2015

BTW, if this PR looks good for merge as-is, I can propose another that moves code like the following into @check_resource (see my line note on docker/client.py above):

    if isinstance(..., dict):
        ... = ....get('Id')

I think those changes are beyond the scope of this PR though.

@shin- shin- changed the title READY FOR REVIEW - Fix #602. Raise ValueError on empty argument to inspect_{container,image}() methods. Raise ValueError on empty argument to inspect_{container,image}() methods. May 20, 2015
@shin- shin- merged commit de2f58d into docker:master May 20, 2015
@shin-
Copy link
Contributor

shin- commented May 20, 2015

Thanks!

@posita posita deleted the raise-on-inspect-empty-string-602 branch May 20, 2015 21:52
@shin- shin- modified the milestone: 1.2.3 Jun 16, 2015
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.

Fixed in #605 - Client.inspect_image("") behaves like Client.images()

2 participants