Bug 1310796 - Review Request: python-etcd - a python client for etcd
Review Request: python-etcd - a python client for etcd
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Zbigniew Jędrzejewski-Szmek
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-22 11:44 EST by Matthew Barnes
Modified: 2016-03-23 15:59 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-03-17 16:51:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zbyszek: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Matthew Barnes 2016-02-22 11:44:51 EST
Spec URL: https://mbarnes.fedorapeople.org/python-etcd/python-etcd.spec
SRPM URL: https://mbarnes.fedorapeople.org/python-etcd/python-etcd-0.4.3-1.fc23.src.rpm

Description:

This is a client library for accessing and manipulating etcd contents from Python.  Needed by Project Atomic.

Fedora Account System Username: mbarnes
Comment 1 Zbigniew Jędrzejewski-Szmek 2016-02-28 14:22:56 EST
Build fails in mock:

======================================================================
ERROR: <nose.suite.ContextSuite context=TestClusterFunctions>
test suite for <class 'etcd.tests.integration.test_simple.TestClusterFunctions'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/nose/suite.py", line 209, in run
    self.setUp()
  File "/usr/lib/python2.7/site-packages/nose/suite.py", line 292, in setUp
    self.setupContext(ancestor)
  File "/usr/lib/python2.7/site-packages/nose/suite.py", line 315, in setupContext
    try_run(context, names)
  File "/usr/lib/python2.7/site-packages/nose/util.py", line 471, in try_run
    return func()
  File "/builddir/build/BUILD/python-etcd-0.4.3/src/etcd/tests/integration/test_simple.py", line 171, in setU
pClass
    program = cls._get_exe()
  File "/builddir/build/BUILD/python-etcd-0.4.3/src/etcd/tests/integration/test_simple.py", line 58, in _get_
exe
    raise Exception('etcd not in path!!')
Exception: etcd not in path!!
Comment 2 Zbigniew Jędrzejewski-Szmek 2016-02-28 14:29:57 EST
Even when etcd is installed in mock, the tests still fail:
======================================================================
FAIL: test_read (etcd.tests.test_auth.EtcdRoleTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/python-etcd-0.4.3/src/etcd/tests/test_auth.py", line 130, in test_read
    self.assertEquals(r.acls, {'*': 'RW'})
AssertionError: {u'/*': 'RW'} != {'*': 'RW'}
- {u'/*': 'RW'}
?  - -
+ {'*': 'RW'}
-------------------- >> begin captured logging << --------------------
urllib3.connectionpool: DEBUG: "GET /v2/auth/users/root HTTP/1.1" 200 33
urllib3.connectionpool: DEBUG: "PUT /v2/auth/users/root HTTP/1.1" 200 33
urllib3.connectionpool: DEBUG: "PUT /v2/auth/users/root HTTP/1.1" 200 27
etcd.client: DEBUG: New etcd client created for http://127.0.0.1:6001
etcd.client: DEBUG: New etcd client created for http://127.0.0.1:6001
urllib3.connectionpool: INFO: Starting new HTTP connection (1): 127.0.0.1
urllib3.connectionpool: DEBUG: "GET /v2/auth/enable HTTP/1.1" 200 18
urllib3.connectionpool: DEBUG: "PUT /v2/auth/enable HTTP/1.1" 200 0
urllib3.connectionpool: DEBUG: "GET /v2/auth/roles/guest HTTP/1.1" 200 69
--------------------- >> end captured logging << ---------------------
----------------------------------------------------------------------
Ran 144 tests in 135.013s
FAILED (failures=1)
Comment 3 Colin Walters 2016-02-29 16:43:59 EST
Can you add a link in the spec to the upstream source?  It is linked from the very bottom of the pypy page, but often when looking at a package I want to know "where do I find the git".  See:

https://fedoraproject.org/wiki/User:Walters/Packaging_VCS_key_proposal
Comment 4 Matthew Barnes 2016-03-01 11:43:49 EST
Updated the spec and srpm.  The tests now pass in F-23 mock for me.

diff --git a/python-etcd.spec b/python-etcd.spec
index 176e270..23f04f8 100644
--- a/python-etcd.spec
+++ b/python-etcd.spec
@@ -10,6 +10,8 @@ License:        MIT
 URL:            http://pypi.python.org/pypi/%{srcname}
 Source0:        https://pypi.python.org/packages/source/p/%{srcname}/%{srcname}-%{version}.tar.gz
·
+#VCS: git:https://github.com/jplana/python-etcd
+
 BuildArch:      noarch
·
 BuildRequires:  python2-devel
@@ -24,6 +26,9 @@ BuildRequires:  python3-mock
 BuildRequires:  python3-nose
 BuildRequires:  python3-pyOpenSSL
·
+# Needed for tests
+BuildRequires:  etcd
+
 %description
 Client library for accessing and manipulating etcd contents.
Comment 5 Zbigniew Jędrzejewski-Szmek 2016-03-01 16:15:24 EST
Still fails in mock, same error as above. I'm using fedora-rawhide-i386 mock.
Comment 6 Matthew Barnes 2016-03-02 11:55:34 EST
(In reply to Zbigniew Jędrzejewski-Szmek from comment #5)
> Still fails in mock, same error as above. I'm using fedora-rawhide-i386 mock.

Okay, I can reproduce that too with fedora-rawhide mock.

Can you verify it works correctly on fedora-23 mock so we're on the same page?
Comment 7 Zbigniew Jędrzejewski-Szmek 2016-03-02 12:52:25 EST
Yeah, it builds fine in fedora-23-i386 mock.
Comment 8 Matthew Barnes 2016-03-02 15:47:16 EST
The test failures are the result of an apparent behavior change between etcd-2.2.1 in F23 and etcd-2.2.5 in Rawhide.

Running the tests from source (pythonX setup.py test) pass for both python2 and python3 on F23, and fail for both python2 and python3 on Rawhide.

I alerted upstream to the problem:
https://github.com/jplana/python-etcd/issues/161

Would it be acceptable to temporarily comment out the %check section (with a link to the issue) for the rawhide branch until this gets fixed upstream?
Comment 9 Zbigniew Jędrzejewski-Szmek 2016-03-04 12:17:12 EST
If it is just this one thing, it'd be more reasonable to patch this one test or filter out this one test from being run. Rawhide/F24 is still very much in flux and a %check section is quite useful.
Comment 10 Matthew Barnes 2016-03-06 21:35:43 EST
Updated the spec and srpm again, and added a patch to work around the test failure.  The patch is kind of lame but works for now.

https://mbarnes.fedorapeople.org/python-etcd/python-etcd-0.4.3-auth-test-fail-workaround.patch

diff --git a/python-etcd.spec b/python-etcd.spec
index 23f04f8..3632570 100644
--- a/python-etcd.spec
+++ b/python-etcd.spec
@@ -29,6 +29,10 @@ BuildRequires:  python3-pyOpenSSL
 # Needed for tests
 BuildRequires:  etcd
 
+BuildRequires:  git
+
+Patch1: python-etcd-0.4.3-auth-test-fail-workaround.patch
+
 %description
 Client library for accessing and manipulating etcd contents.
 
@@ -51,7 +55,7 @@ Requires:       python3-dns
 Client library for accessing and manipulating etcd contents.
 
 %prep
-%autosetup
+%autosetup -Sgit
 
 %build
 %py2_build
Comment 11 Zbigniew Jędrzejewski-Szmek 2016-03-06 22:17:37 EST
BR:git-core is less costly than BR:git. But in fact I don't think you need the BR and -Sgit at all, things usually work with plain patch.

The %description is a better summary than the Summary, and the Summary itself seems wrong. I think you should move the %description text into Summary, and maybe add some more details in the %description if possible. Also, there's no need to define %sum, you can put whatever text in the first Summary, and in subsequent ones use %summary to refer to the previous one.

+ latest version
+ license is acceptable (MIT)
- license file is not present

Please package the license file and use %license. I see that the pypi tarball doesn't have a license, upstream screw that up quite often. You can use the github tarball instead.

+ provides/requires look OK
+ %python_provide is used
+ no scriptlets required or necessary

rpmlint complains:
python-etcd.src:74: W: macro-in-comment %{__python3}

You should fix that (%%) because otherwise rpm complains on every build.

Looks good otherwise.
Comment 12 Colin Walters 2016-03-07 10:18:13 EST
(In reply to Zbigniew Jędrzejewski-Szmek from comment #11)
> BR:git-core is less costly than BR:git. But in fact I don't think you need
> the BR and -Sgit at all, things usually work with plain patch.

I personally always use -Sgit even without patches, because in the scenario where I do need to add one, it's much less painful.
Comment 13 Matthew Barnes 2016-03-07 11:34:00 EST
Well the Summary is exactly the author's own description.

  see https://github.com/jplana/python-etcd
  and https://pypi.python.org/pypi/python-etcd

I added the word "library" for clarification, and tried to expand on the description.

Think I got everything else you noted.

diff --git a/python-etcd.spec b/python-etcd.spec
index 3632570..0efb666 100644
--- a/python-etcd.spec
+++ b/python-etcd.spec
@@ -1,14 +1,16 @@
 %global srcname python-etcd
-%global sum A python client for etcd
 
 Name:           %{srcname}
 Version:        0.4.3
 Release:        1%{?dist}
-Summary:        %{sum}
+Summary:        A python client library for etcd
 
 License:        MIT
 URL:            http://pypi.python.org/pypi/%{srcname}
-Source0:        https://pypi.python.org/packages/source/p/%{srcname}/%{srcname}
+
+# Using the github URL because the tarball file at pypi excludes
+# the license file. But github tarball files are named awkwardly.
+Source0:        https://github.com/jplana/%{srcname}/archive/%{version}.tar.gz
 
 #VCS: git:https://github.com/jplana/python-etcd
 
@@ -29,33 +31,40 @@ BuildRequires:  python3-pyOpenSSL
 # Needed for tests
 BuildRequires:  etcd
 
-BuildRequires:  git
-
 Patch1: python-etcd-0.4.3-auth-test-fail-workaround.patch
 
 %description
-Client library for accessing and manipulating etcd contents.
+Client library for interacting with an etcd service, providing Python
+access to the full etcd REST API.  Includes authentication, accessing
+and manipulating shared content, managing cluster members, and leader
+election.
 
 %package -n python2-%{srcname}
-Summary:        %{sum}
+Summary:        %summary
 Requires:       etcd
 Requires:       python-dns
 %{?python_provide:%python_provide python2-%{srcname}}
 
 %description -n python2-%{srcname}
-Client library for accessing and manipulating etcd contents.
+Client library for interacting with an etcd service, providing Python
+access to the full etcd REST API.  Includes authentication, accessing
+and manipulating shared content, managing cluster members, and leader
+election.
 
 %package -n python3-%{srcname}
-Summary:        %{sum}
+Summary:        %summary
 Requires:       etcd
 Requires:       python3-dns
 %{?python_provide:%python_provide python3-%{srcname}}
 
 %description -n python3-%{srcname}
-Client library for accessing and manipulating etcd contents.
+Client library for interacting with an etcd service, providing Python
+access to the full etcd REST API.  Includes authentication, accessing
+and manipulating shared content, managing cluster members, and leader
+election.
 
 %prep
-%autosetup -Sgit
+%autosetup -p1
 
 %build
 %py2_build
@@ -71,10 +80,11 @@ Client library for accessing and manipulating etcd contents.
 # This seems to require a newer python3-mock than what's currently available
 # in F23, and even Rawhide.  If I let it download mock-1.3.0 from the Python
 # Package Index (pypi) then tests pass.
-#%{__python3} setup.py test
+#%%{__python3} setup.py test
 
 %files -n python2-%{srcname}
 %doc README.rst
+%license LICENSE.txt
 %{python2_sitelib}/*
 
 %files -n python3-%{srcname}
Comment 14 Matthew Barnes 2016-03-07 11:45:38 EST
Realized I forgot to add LICENSE.txt to the python3 package.  Fixed now.
Comment 15 Zbigniew Jędrzejewski-Szmek 2016-03-07 12:10:23 EST
Package is APPROVED.
Comment 16 Matthew Barnes 2016-03-07 12:36:52 EST
Thanks!
Comment 17 Zbigniew Jędrzejewski-Szmek 2016-03-07 13:33:54 EST
I forgot to add: you can append "#/%{name}-%{commit}.tar.gz" or similar to Source0, and spectool -g will then use this part after the slash as the file name.
Comment 18 Gwyn Ciesla 2016-03-07 14:09:13 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/python-etcd
Comment 19 Fedora Update System 2016-03-08 17:20:34 EST
python-etcd-0.4.3-1.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-cbd4c896de
Comment 20 Fedora Update System 2016-03-09 15:58:28 EST
python-etcd-0.4.3-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-5917b2c8ab
Comment 21 Fedora Update System 2016-03-09 17:55:24 EST
python-etcd-0.4.3-1.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-cbd4c896de
Comment 22 Fedora Update System 2016-03-09 20:54:42 EST
python-etcd-0.4.3-1.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-5917b2c8ab
Comment 23 Fedora Update System 2016-03-17 16:51:32 EDT
python-etcd-0.4.3-1.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
Comment 24 Fedora Update System 2016-03-23 15:59:33 EDT
python-etcd-0.4.3-1.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

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