Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.

Bug 1310796

Summary: Review Request: python-etcd - a python client for etcd
Product: [Fedora] Fedora Reporter: Matthew Barnes <mbarnes>
Component: Package ReviewAssignee: Zbigniew Jędrzejewski-Szmek <zbyszek>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: package-review, walters, zbyszek
Target Milestone: ---Keywords: Reopened
Target Release: ---Flags: zbyszek: fedora-review+
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-03-17 20:51:34 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Matthew Barnes 2016-02-22 16:44:51 UTC
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 19:22:56 UTC
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 19:29:57 UTC
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 21:43:59 UTC
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 16:43:49 UTC
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 21:15:24 UTC
Still fails in mock, same error as above. I'm using fedora-rawhide-i386 mock.

Comment 6 Matthew Barnes 2016-03-02 16:55:34 UTC
(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 17:52:25 UTC
Yeah, it builds fine in fedora-23-i386 mock.

Comment 8 Matthew Barnes 2016-03-02 20:47:16 UTC
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 17:17:12 UTC
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-07 02:35:43 UTC
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-07 03:17:37 UTC
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 15:18:13 UTC
(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 16:34:00 UTC
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 16:45:38 UTC
Realized I forgot to add LICENSE.txt to the python3 package.  Fixed now.

Comment 15 Zbigniew Jędrzejewski-Szmek 2016-03-07 17:10:23 UTC
Package is APPROVED.

Comment 16 Matthew Barnes 2016-03-07 17:36:52 UTC
Thanks!

Comment 17 Zbigniew Jędrzejewski-Szmek 2016-03-07 18:33:54 UTC
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 19:09:13 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/python-etcd

Comment 19 Fedora Update System 2016-03-08 22:20:34 UTC
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 20:58:28 UTC
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 22:55:24 UTC
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-10 01:54:42 UTC
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 20:51:32 UTC
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 19:59:33 UTC
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.