Bug 1367769

Summary: Review Request: loopabull - Event loop driven Ansible playbook execution engine
Product: [Fedora] Fedora Reporter: Adam Miller <admiller>
Component: Package ReviewAssignee: Igor Gnatenko <ignatenko>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: package-review
Target Milestone: ---Flags: ignatenko: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-11-14 12:48:41 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:
Attachments:
Description Flags
proposed patch none

Description Adam Miller 2016-08-17 12:27:45 UTC
Spec URL: https://maxamillion.fedorapeople.org/loopabull.spec
SRPM URL: https://maxamillion.fedorapeople.org/loopabull-0.0.3-1.fc24.src.rpm
Description: Event loop driven Ansible playbook execution engine
Fedora Account System Username: maxamillion

Comment 1 Adam Miller 2016-08-17 12:28:35 UTC
I have it building in a COPR here:

https://copr.fedorainfracloud.org/coprs/maxamillion/loopabull/

Comment 2 Igor Gnatenko 2016-08-17 13:18:19 UTC
> %autosetup -n %{name}-%{version}
%autosetup

> %{__python2} setup.py build
%py2_build

> %{__python2} setup.py install -O1 --skip-build --root $RPM_BUILD_ROOT
%py2_install

> %description
> %{summary}
%description
%{summary}.

> gzip man/%{name}.1
> install -d %{buildroot}%{_mandir}/man1
> install -m 644 man/%{name}.1.gz %{buildroot}%{_mandir}/man1/
> %{_mandir}/man1/%{name}.1.gz
install -Dpm0644 man/%{name}.1 %{buildroot}%{_mandir}/man1/%{name}.1
%doc %{_mandir}/man1/%{name}.1*

> # Install examples
> install -d %{buildroot}%{_docdir}/%{name}/
> cp -r examples %{buildroot}%{_docdir}/%{name}/
> %{_docdir}/%{name}/*
%doc examples

> Source0:        https://pypi.python.org/packages/source/l/%{name}/%{name}-%{version}.tar.gz
is it really necessary to take from PyPI and not from GitHub? Also that link is broken anyway.

> BuildRequires:  systemd
BuildRequires: systemd-units

Comment 3 Adam Miller 2016-08-17 16:20:44 UTC
systemd-units got merged into the systemd package a little while back according to the guidelines: https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations

Thank you for the review! I fixed everything else mentioned and look forward to further feedback.

Spec URL: https://maxamillion.fedorapeople.org/loopabull.spec
SRPM URL: https://maxamillion.fedorapeople.org/loopabull-0.0.3-2.fc24.src.rpm

Comment 4 Igor Gnatenko 2016-08-17 17:58:13 UTC
(In reply to Adam Miller from comment #3)
> systemd-units got merged into the systemd package a little while back
> according to the guidelines:
> https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations
Ah, I didn't read that carefully. sorry.
> 
> Thank you for the review! I fixed everything else mentioned and look forward
> to further feedback.
> 
> Spec URL: https://maxamillion.fedorapeople.org/loopabull.spec
> SRPM URL: https://maxamillion.fedorapeople.org/loopabull-0.0.3-2.fc24.src.rpm

> Source0:        https://github.com/maxamillion/%{name}/archive/%{name}-%{version}.tar.gz
> %autosetup -n %{name}-%{name}-%{version}
-> Source0:        %{url}/archive/%{version}/%{name}-%{version}.tar.gz
-> %autosetup
and it will not prepend any name

* actually manpages stuff is still not fixed
* add "-p" flag to `install` to preserver timestamps
* %{_docdir}/%{name}/* is still there

Comment 5 Igor Gnatenko 2016-08-17 18:05:06 UTC
Created attachment 1191708 [details]
proposed patch

Comment 6 Adam Miller 2016-08-17 20:14:21 UTC
+1 to most of the patch, but as is it doesn't build because of the extra preprended %{name} from the GitHub archive. I changed just the %autosetup line and I added a changelog with credit for the patch (I'll add your email or fas account info if you like.

Spec URL: https://maxamillion.fedorapeople.org/loopabull.spec
SRPM URL: https://maxamillion.fedorapeople.org/loopabull-0.0.3-3.fc24.src.rpm

Comment 7 Igor Gnatenko 2016-08-18 06:05:08 UTC
(In reply to Adam Miller from comment #6)
> +1 to most of the patch, but as is it doesn't build because of the extra
> preprended %{name} from the GitHub archive. I changed just the %autosetup
> line and I added a changelog with credit for the patch (I'll add your email
> or fas account info if you like.
> 
> Spec URL: https://maxamillion.fedorapeople.org/loopabull.spec
> SRPM URL: https://maxamillion.fedorapeople.org/loopabull-0.0.3-3.fc24.src.rpm

Looks like you didn't see change in Source0 line.
If you put there: %{URL}/archive/%{version}/%{name}-%(version}.tar.gz
Then it will not prepend extra name (problem is that you already had old archive with same filename).

Comment 8 Adam Miller 2016-08-18 21:53:57 UTC
Ah, sure enough. Fixed, thanks!

Comment 9 Gwyn Ciesla 2016-08-19 13:44:57 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/loopabull