Bug 1048667 - Review Request: python-docker-py - An API client for docker written in Python
Summary: Review Request: python-docker-py - An API client for docker written in Python
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Lokesh Mandvekar
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1049424
Blocks: DebugInfo
TreeView+ depends on / blocked
 
Reported: 2014-01-06 04:54 UTC by Lokesh Mandvekar
Modified: 2014-08-05 14:17 UTC (History)
9 users (show)

Fixed In Version: python-docker-py-0.2.3-8.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-02-25 07:48:35 UTC
Type: ---
bkabrda: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Lokesh Mandvekar 2014-01-06 04:54:59 UTC
Spec URL: http://fedorapeople.org/cgit/lsm5/public_git/docker-py.git/plain/SOURCES/docker-py.spec
SRPM URL: http://fedorapeople.org/cgit/lsm5/public_git/docker-py.git/tree/SRPMS/docker-py-0.2.3-1.fc21.src.rpm
Description: An API client for docker written in Python
Fedora Account System Username: lsm5

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6363131

$ rpmlint SOURCES/docker-py.spec SRPMS/docker-py-0.2.3-1.fc21.src.rpm RPMS/x86_64/docker-py-devel-0.2.3-1.fc21.x86_64.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 1 Lokesh Mandvekar 2014-01-06 05:02:02 UTC
Vincent, hope you don't mind me assigning it to you, feel free to reset it to default assignee :), if anyone else wants to pick it up for review, feel free to jump in :), thanks!

Comment 2 Bohuslav "Slavek" Kabrda 2014-01-06 10:22:48 UTC
Few fly-by comments (I can take the full review if that's ok with you, I really want this package in Fedora):
- You should consider running tests in %check section during build.
- Why is there only the -devel package? It doesn't make any sense to me. Why not keep everything in the main package?
- You have many BuildRequires for python packages, but no Requires (runtime deps) for those. Why? Are they not needed? It doesn't seem so.
- Could you please consider adding python3- subpackage?
- AFAIK it'd be best to use PyPI as upstream [1], since it provides nice urls and source archive that is actually named "docker-py-0.2.3.tar.gz" (whereas the one from GH is named only "0.2.3.tar.gz").
- You should name the package python-docker-py. This naming convention was agreed on at [2].
- Unless you want to build this for epel, please drop the %defattr line from %files, it's not needed for Fedora anymore.
- It seems to me that the package doesn't actually own %{python_sitelib}/docker directory.

[1] https://pypi.python.org/pypi/docker-py
[2] https://fedorahosted.org/fpc/ticket/271

Comment 3 Lokesh Mandvekar 2014-01-06 15:18:58 UTC
Thanks Slavek for offering to take this up for review, I'm assigning the review to you and I'll address your comments in a bit

Comment 4 Lokesh Mandvekar 2014-01-06 17:18:06 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #2)
> - You should name the package python-docker-py. This naming convention was
> agreed on at [2].

> [2] https://fedorahosted.org/fpc/ticket/271

Before I post my revised files addressing other comments, wouldn't python-docker-py sound kinda redundant? IMHO, python-docker would sound much better :) (?)

Comments?

Comment 5 Lokesh Mandvekar 2014-01-06 23:29:09 UTC
Spec URL: http://fedorapeople.org/cgit/lsm5/public_git/python-docker.git/plain/SOURCES/python-docker.spec
SRPM URL: http://fedorapeople.org/cgit/lsm5/public_git/python-docker.git/tree/SRPMS/python-docker-0.2.3-2.fc21.src.rpm

(In reply to Bohuslav "Slavek" Kabrda from comment #2)
> Few fly-by comments (I can take the full review if that's ok with you, I
> really want this package in Fedora):
> - You should consider running tests in %check section during build.

Added but commented out for now, it complains python-six version mismatch, requirements.txt looks for six==1.3.0 but installed is 1.4.1. Any suggestions on overriding/patching/ignoring?

> - Why is there only the -devel package? It doesn't make any sense to me. Why
> not keep everything in the main package?

When I only had the main package, rpmlint complained E: no-binary. That's why I put stuff in devel. Let me know if it should still go in the main package.

> - You have many BuildRequires for python packages, but no Requires (runtime
> deps) for those. Why? Are they not needed? It doesn't seem so.

Checkout the new build and runtime deps

> - Could you please consider adding python3- subpackage?

Added. btw, 'import docker' on python3 fails. It says: NameError: name 'http' is not defined, probably because python-websocket-client isn't built for python3, we gotta request for that as well.

> - AFAIK it'd be best to use PyPI as upstream [1], since it provides nice
> urls and source archive that is actually named "docker-py-0.2.3.tar.gz"
> (whereas the one from GH is named only "0.2.3.tar.gz").

Done.

> - You should name the package python-docker-py. This naming convention was
> agreed on at [2].

Currently at python-docker, but I'll rename it to python-docker-py if you still think latter is better.

> - Unless you want to build this for epel, please drop the %defattr line from
> %files, it's not needed for Fedora anymore.

Yes, I'll be building for epel too. I added a conditional to check for epel, check that out as well.

> - It seems to me that the package doesn't actually own
> %{python_sitelib}/docker directory.

Done.

> 
> [1] https://pypi.python.org/pypi/docker-py
> [2] https://fedorahosted.org/fpc/ticket/271

Comment 6 Bohuslav "Slavek" Kabrda 2014-01-07 10:07:51 UTC
- The reason of mandating the "python-" prefix rule is, that packages in Fedora should have predictable names derived from upstream names. Previously, there were three naming schemes that different packagers used for their packages:
 * docker-py
 * python-docker
 * python-docker-py
This led to unpredictable names. I was hitting this issue from time to time, I had to try 3 yum installs or do yum search before knowing what to install, which took time. Then we agreed that we will have all the new packages named "python-%{pypi_name}", whatever "%{pypi_name}" is. So the reason for the rule is having predictable package names, therefore you should use python-docker-py. I hope that makes sense.
- You should really put the stuff in the main package, it's the standard approach for python extension modules. I never see the error you mentioned, but I'll investigate it when you move the files. If rpmlint complains about this, then it's a bug in rpmlint, not in your packaging :)
- Please request building python-websocket-client for python3, we can't push the subpackage without dependencies.
- As for the tests, have you tried removing the requirements.txt prior to executing the test suite? That should help, IMHO.
- Your runtime Requires are wrong. All the "Requires:" for python3 packages should go to the python3- subpackage (which should not be -devel, too) + you should also put Requires: docker-io to the subpackage. With your solution, the python2-docker package would depend on all the python3- dependencies, but the python3-docker package wouldn't, so you need to move them. The python- and python3- packages just have to be independent of each other.
- You don't have to put the %defattr line in a rhel-6 condition. It won't do no harm on Fedora. My note was just that if you do not mean to build in EPEL, you won't need it at all. Since clearly you want to build in EPEL, just leave it there as it was previously.

Thanks for your work on this!

Comment 7 Lokesh Mandvekar 2014-01-07 15:30:32 UTC
* Tue Jan 07 2014 Lokesh Mandvekar <lsm5@redhat.com> 0.2.3-3
- Everything goes in main package
- python3 package requires corrected
- package name python-docker-py
- both packages require docker-io

Spec URL: http://lsm5.fedorapeople.org/python-docker-py/SOURCES/python-docker-py.spec
SRPM URL: http://lsm5.fedorapeople.org/python-docker-py/SRPMS/python-docker-py-0.2.3-3.fc21.src.rpm

(In reply to Bohuslav "Slavek" Kabrda from comment #6)
> - The reason of mandating the "python-" prefix rule is, that packages in
> Fedora should have predictable names derived from upstream names.
> Previously, there were three naming schemes that different packagers used
> for their packages:
>  * docker-py
>  * python-docker
>  * python-docker-py
> This led to unpredictable names. I was hitting this issue from time to time,
> I had to try 3 yum installs or do yum search before knowing what to install,
> which took time. Then we agreed that we will have all the new packages named
> "python-%{pypi_name}", whatever "%{pypi_name}" is. So the reason for the
> rule is having predictable package names, therefore you should use
> python-docker-py. I hope that makes sense.

Done.

> - You should really put the stuff in the main package, it's the standard
> approach for python extension modules. I never see the error you mentioned,
> but I'll investigate it when you move the files. If rpmlint complains about
> this, then it's a bug in rpmlint, not in your packaging :)

Done.

> - Please request building python-websocket-client for python3, we can't push
> the subpackage without dependencies.

Done.

> - As for the tests, have you tried removing the requirements.txt prior to
> executing the test suite? That should help, IMHO.

setup.py seems to use requirements.txt so deleting that file complains. Should I request upstream to use >= instead of == ?

> - Your runtime Requires are wrong. All the "Requires:" for python3 packages
> should go to the python3- subpackage (which should not be -devel, too) + you
> should also put Requires: docker-io to the subpackage. With your solution,
> the python2-docker package would depend on all the python3- dependencies,
> but the python3-docker package wouldn't, so you need to move them. The
> python- and python3- packages just have to be independent of each other.

Done.

> - You don't have to put the %defattr line in a rhel-6 condition. It won't do
> no harm on Fedora. My note was just that if you do not mean to build in
> EPEL, you won't need it at all. Since clearly you want to build in EPEL,
> just leave it there as it was previously.

Done.
> 
> Thanks for your work on this!

Comment 8 Bohuslav "Slavek" Kabrda 2014-01-07 16:23:00 UTC
(In reply to Lokesh Mandvekar from comment #7)
> > - As for the tests, have you tried removing the requirements.txt prior to
> > executing the test suite? That should help, IMHO.
> 
> setup.py seems to use requirements.txt so deleting that file complains.
> Should I request upstream to use >= instead of == ?

Oh, I didn't notice that the upstream doesn't ship the test suite... So it would be ideal if you could ask them to ship the test suite and to relax dependencies. If they won't agree to relaxing dependencies, I'd suggest patching setup.py (emptying the requirements list), so that it doesn't complain.

So for this review, let's just skip the tests and I'll leave it up to you to add them there when upstream releases new version (BTW for commented macros, you should double all the percent signs, as noted in [1] (rpmlint complains about that, too)). Note that you'll also need to add all the Requires as BuildRequires, too, since they'll be needed for tests (but are not needed now).


Some more comments:
- The section "%description -n python3-docker-py" should be in the with_python3 conditional, too.
- The rpmlint "no binary" and "empty debuginfo package" are sort of an rpmlint bug. It doesn't expect ExclusiveArch packages to not contain any binary code (and thus not producing any debuginfo packages), but due to docker-io limitation to x86_64 we can ignore it in this case.
- The only "really blocking" problem is now having "python3-websocket-client". So when you fix the description as mentioned above and python3-websocket-client package is added, I'll approve this, everything else looks fine now.

[1] https://fedoraproject.org/wiki/How_to_create_an_RPM_package#SPEC_file_overview

Comment 9 Lokesh Mandvekar 2014-01-09 14:15:50 UTC
%changelog
* Thu Jan 07 2014 Lokesh Mandvekar <lsm5@redhat.com> 0.2.3-5
- python3 to be added after python3-websocket-client (BZ 1049424)


Spec URL: http://lsm5.fedorapeople.org/python-docker-py/SOURCES/python-docker-py.spec
SRPM URL: http://lsm5.fedorapeople.org/python-docker-py/SRPMS/python-docker-py-0.2.3-5.fc21.src.rpm

python2 only for now.

Also, I see that by the time it comes to %check, it looks for the tests directory but can't seem to find it (ImportError: No module named tests), I'm trying to figure out why it gets deleted.

Comment 10 Bohuslav "Slavek" Kabrda 2014-01-09 14:21:37 UTC
As noted in comment #8, I noticed that upstream doesn't ship tests, unfortunately. So it is ok to just remove the %check section, but it'd be nice to ask them to ship the tests (sorry I didn't notice this earlier before... I just looked upstream and not in the tarball).

So this package seems OK now, APPROVED.

Thanks!

Comment 11 Lokesh Mandvekar 2014-01-09 14:24:07 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #10)
> As noted in comment #8, I noticed that upstream doesn't ship tests,
> unfortunately. So it is ok to just remove the %check section, but it'd be
> nice to ask them to ship the tests (sorry I didn't notice this earlier
> before... I just looked upstream and not in the tarball).

Great, I'll talk to them.
> 
> So this package seems OK now, APPROVED.

woohoo.. awesome!
> 
> Thanks!

Thanks for the review :)

Comment 12 Lokesh Mandvekar 2014-01-09 14:26:47 UTC
New Package SCM Request
=======================
Package Name: python-docker-py
Short Description: An API client for docker written in Python
Owners: lsm5
Branches: f19 f20 el6
InitialCC:

Comment 13 Gwyn Ciesla 2014-01-09 14:39:36 UTC
Git done (by process-git-requests).

Comment 14 Fedora Update System 2014-01-09 15:15:01 UTC
python-docker-py-0.2.3-5.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-5.fc20

Comment 15 Fedora Update System 2014-01-09 15:24:14 UTC
python-docker-py-0.2.3-5.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-5.fc19

Comment 16 Fedora Update System 2014-01-09 15:33:43 UTC
python-docker-py-0.2.3-5.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-5.el6

Comment 17 Fedora Update System 2014-01-10 01:08:52 UTC
python-docker-py-0.2.3-5.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 18 Ville Skyttä 2014-01-14 16:16:05 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #8)
> - The rpmlint "no binary" and "empty debuginfo package" are sort of an
> rpmlint bug.

The latter isn't. Empty debuginfo packages are useless so they should be explicitly disabled for packages that can't be made noarch.

Comment 19 Bohuslav "Slavek" Kabrda 2014-01-15 07:06:06 UTC
(In reply to Ville Skyttä from comment #18)
> (In reply to Bohuslav "Slavek" Kabrda from comment #8)
> > - The rpmlint "no binary" and "empty debuginfo package" are sort of an
> > rpmlint bug.
> 
> The latter isn't. Empty debuginfo packages are useless so they should be
> explicitly disabled for packages that can't be made noarch.

Ah, I wasn't aware of that. Lokesh, please consider doing this when updating the package next time. Thanks.

Comment 20 Marek Goldmann 2014-02-07 09:36:34 UTC
The "Requires: python-mock" should be removed too, this is only a build time dependency.

Comment 21 Fedora Update System 2014-02-07 15:59:12 UTC
python-docker-py-0.2.3-6.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-6.el6

Comment 22 Fedora Update System 2014-02-07 16:00:35 UTC
python-docker-py-0.2.3-6.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-6.fc20

Comment 23 Fedora Update System 2014-02-07 16:01:30 UTC
python-docker-py-0.2.3-6.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-6.fc19

Comment 24 Ville Skyttä 2014-02-07 21:31:22 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #19)
> (In reply to Ville Skyttä from comment #18)
> > (In reply to Bohuslav "Slavek" Kabrda from comment #8)
> > > - The rpmlint "no binary" and "empty debuginfo package" are sort of an
> > > rpmlint bug.
> > 
> > The latter isn't. Empty debuginfo packages are useless so they should be
> > explicitly disabled for packages that can't be made noarch.
> 
> Ah, I wasn't aware of that. Lokesh, please consider doing this when updating
> the package next time. Thanks.

This is still unfixed in 0.2.3-6.

Comment 25 Lokesh Mandvekar 2014-02-07 21:52:29 UTC
(In reply to Ville Skyttä from comment #24)
> (In reply to Bohuslav "Slavek" Kabrda from comment #19)
> > (In reply to Ville Skyttä from comment #18)
> > > (In reply to Bohuslav "Slavek" Kabrda from comment #8)
> > > > - The rpmlint "no binary" and "empty debuginfo package" are sort of an
> > > > rpmlint bug.
> > > 
> > > The latter isn't. Empty debuginfo packages are useless so they should be
> > > explicitly disabled for packages that can't be made noarch.
> > 
> > Ah, I wasn't aware of that. Lokesh, please consider doing this when updating
> > the package next time. Thanks.
> 
> This is still unfixed in 0.2.3-6.

--ohh shoot ...i'll push out another update in a bit. Apologies about that.

Comment 26 Fedora Update System 2014-02-08 04:59:36 UTC
Package python-docker-py-0.2.3-6.fc20:
* should fix your issue,
* was pushed to the Fedora 20 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing python-docker-py-0.2.3-6.fc20'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2014-2147/python-docker-py-0.2.3-6.fc20
then log in and leave karma (feedback).

Comment 27 Fedora Update System 2014-02-08 20:40:31 UTC
python-docker-py-0.2.3-7.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-7.el6

Comment 28 Fedora Update System 2014-02-08 20:41:23 UTC
python-docker-py-0.2.3-7.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-7.fc20

Comment 29 Fedora Update System 2014-02-08 20:42:00 UTC
python-docker-py-0.2.3-7.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-7.fc19

Comment 30 Lokesh Mandvekar 2014-02-08 20:49:42 UTC
All: Let me know how the latest release looks. Also, feel free to add yourself as co-maintainers :)

Comment 31 Fedora Update System 2014-02-10 19:02:05 UTC
python-docker-py-0.2.3-8.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-8.el6

Comment 32 Fedora Update System 2014-02-10 19:18:44 UTC
python-docker-py-0.2.3-8.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-8.fc20

Comment 33 Fedora Update System 2014-02-10 19:19:24 UTC
python-docker-py-0.2.3-8.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-docker-py-0.2.3-8.fc19

Comment 34 Fedora Update System 2014-02-25 07:48:35 UTC
python-docker-py-0.2.3-8.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 35 Fedora Update System 2014-02-25 07:50:43 UTC
python-docker-py-0.2.3-8.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 36 Fedora Update System 2014-03-06 21:08:00 UTC
python-docker-py-0.2.3-8.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 37 fortuik 2014-08-05 14:17:20 UTC
Hello,
any chance to push docker-py to the rhel-7-server-extras-rpms?
It would be really useful to have the python api client on the HOST system.

If I got it right, docker is now a core component of RHEL7 and consequently the EPEL7 docker-io packages are not maintained.
Any chance to remove the EPEL7 obsoleted ocker packages, it is a bit strange?

I'm sorry if this is not the appropriate place to raise this.


Note You need to log in before you can comment on or make changes to this bug.