Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1694278 - Review Request: python-fastpurge - A Python client for the Akamai Fast Purge API
Summary: Review Request: python-fastpurge - A Python client for the Akamai Fast Purge API
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Robert-André Mauchin
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-03-30 01:01 UTC by Rohan McGovern
Modified: 2019-04-06 02:31 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-04-06 02:31:25 UTC
zebob.m: fedora-review+


Attachments (Terms of Use)

Description Rohan McGovern 2019-03-30 01:01:53 UTC
Spec URL: https://fedorapeople.org/~rohanpm/python-fastpurge/1/python-fastpurge.spec
SRPM URL: https://fedorapeople.org/~rohanpm/python-fastpurge/1/python-fastpurge-1.0.2-1.fc31.src.rpm

Description:

This library provides a simple asynchronous Python wrapper for the Fast
Purge API, including authentication and error recovery.

Fedora Account System Username: rohanpm

Comment 1 Robert-André Mauchin 2019-04-01 19:10:49 UTC
 Consider running the tests:

Source0: %url/archive/v%{version}/%{srcname}-%{version}.tar.gz

[…]

BuildRequires:	python3dist(pytest)
BuildRequires:	python3dist(akamai.edgegrid)
BuildRequires:	python3dist(more_executors)
BuildRequires:	python3dist(monotonic)
BuildRequires:	python3dist(mock)
BuildRequires:	python3dist(requests-mock)

[…]

%prep
%autosetup -n %{name}-%{version}

[…]

%check
%{__python3} -m pytest -v


You'll need to package python-akamai-edgegrid and python-more_executors, which you should do anyway otherwise your package can't work.

Comment 2 Rohan McGovern 2019-04-02 08:08:12 UTC
> Consider running the tests:
> 
> Source0: %url/archive/v%{version}/%{srcname}-%{version}.tar.gz

The tests aren't included in the tarball shipped to PyPI, I guess that may be why you've suggested a Source0 pointing at github there.

Should we point Source* at github archive actually? I've released things that way in the past but started to become uncomfortable with it, not knowing whether that's stable and not having control over it.

I just did a quick search about this and it seems indeed there *have* been incidents where github archive contents suddenly changed, e.g. https://github.com/spack/spack/issues/5411.  So today's Source0 might be unreproducible tomorrow.

If we'd want to use %check here, I guess I should rather have the tests included in the upstream releases to PyPI.

> BuildRequires:	python3dist(akamai.edgegrid)
> BuildRequires:	python3dist(more_executors)

On a related note, is there any more guidance about when %check should be used? https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites is so terse.  I don't know any concrete issue with it, but I felt that adding almost all Requires also into BuildRequires for running the test suite seems weird and I was worried there could be some negative effects to this.

> You'll need to package python-akamai-edgegrid and python-more_executors,
> which you should do anyway otherwise your package can't work.

Yep, they are present (in rawhide).  Built python3-fastpurge RPM is installable in latest fedora:rawhide container image.

Comment 3 Robert-André Mauchin 2019-04-02 13:32:02 UTC
We have no other choice than to trust that upstream won't rewrite their Github history. Thousands of packages already depend on Github archive links. Source0 will be uploaded to dist-git once you import the package, after that it will stay the same in Fedora repos.

> I don't know any concrete issue with it, but I felt that adding almost all Requires also into BuildRequires for running the test suite seems weird and I was worried there could be some negative effects to this.

It is not weird, it is the correct procedure to test if the package is working as expected.

> Yep, they are present (in rawhide).  Built python3-fastpurge RPM is installable in latest fedora:rawhide container image.

Ha I see, I tried searching for akamai instead of edgegrid and couldn't find the dep.

BR should then be:

BuildRequires:	python3dist(edgegrid-python)
BuildRequires:	python3dist(more-executors)


Tests run fine:
+ /usr/bin/python3 -m pytest -v
============================= test session starts ==============================
platform linux -- Python 3.7.3, pytest-3.9.3, py-1.7.0, pluggy-0.8.1 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /builddir/build/BUILD/python-fastpurge-1.0.2, inifile:
plugins: requests-mock-1.5.2
collecting ... collected 10 items
tests/test_auth_dict.py::test_auth_from_dict PASSED                      [ 10%]
tests/test_auth_dict.py::test_auth_bad_type PASSED                       [ 20%]
tests/test_auth_dict.py::test_auth_missing_settings PASSED               [ 30%]
tests/test_auth_dict.py::test_auth_from_home_edgerc PASSED               [ 40%]
tests/test_auth_dict.py::test_auth_from_custom_edgerc PASSED             [ 50%]
tests/test_purge.py::test_purge_by_url PASSED                            [ 60%]
tests/test_purge.py::test_purge_by_tag PASSED                            [ 70%]
tests/test_purge.py::test_scheme_port PASSED                             [ 80%]
tests/test_purge.py::test_response_fails PASSED                          [ 90%]
tests/test_purge.py::test_split_requests PASSED                          [100%]
========================== 10 passed in 6.60 seconds ===========================
+ exit 0

I strongly suggest you add them. Maybe you could convince upstream to add them to the Pypi archive for the next release if you prefer.

Comment 4 Rohan McGovern 2019-04-04 21:43:26 UTC
Thank you.  I looked into other Python packages with %check and they're exactly in line with your suggestions too.

Updated package:

Spec URL: https://fedorapeople.org/~rohanpm/python-fastpurge/2/python-fastpurge.spec
SRPM URL: https://fedorapeople.org/~rohanpm/python-fastpurge/2/python-fastpurge-1.0.2-2.fc31.src.rpm

Changes:

- sources now come from github (for test suite)
- run test suite in %check

Comment 5 Rohan McGovern 2019-04-04 21:49:42 UTC
I've noticed one rpmlint complaint now:

  python-fastpurge.src: W: invalid-url Source0: %url/archive/v1.0.2/fastpurge-1.0.2.tar.gz
  1 packages and 0 specfiles checked; 0 errors, 1 warnings.

It seems to work fine despite this, but I could fix the warning by moving URL prior to Source0.

Comment 6 Robert-André Mauchin 2019-04-04 21:53:57 UTC
(In reply to Rohan McGovern from comment #5)
> I've noticed one rpmlint complaint now:
> 
>   python-fastpurge.src: W: invalid-url Source0:
> %url/archive/v1.0.2/fastpurge-1.0.2.tar.gz
>   1 packages and 0 specfiles checked; 0 errors, 1 warnings.
> 
> It seems to work fine despite this, but I could fix the warning by moving
> URL prior to Source0.

Maybe it should be:

Source0: %{url}/archive/v%{version}/%{name}-%{version}.tar.gz

Anyhow, package approved.

Comment 7 Gwyn Ciesla 2019-04-05 13:33:59 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/python-fastpurge

Comment 8 Rohan McGovern 2019-04-06 02:31:25 UTC
Built for rawhide: https://koji.fedoraproject.org/koji/buildinfo?buildID=1244929


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