Bug 1205489 - Review Request: edeploy - Linux system provisioning and upgrades made easy
Summary: Review Request: edeploy - Linux system provisioning and upgrades made easy
Keywords:
Status: CLOSED EOL
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Haïkel Guémar
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1224513
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-03-25 03:03 UTC by Frédéric Lepied
Modified: 2019-07-31 13:58 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2019-07-31 13:58:54 UTC
Type: ---
Embargoed:
karlthered: fedora-review-


Attachments (Terms of Use)

Description Frédéric Lepied 2015-03-25 03:03:06 UTC
Spec URL: https://flepied.fedorapeople.org/edeploy.spec
SRPM URL: https://flepied.fedorapeople.org/edeploy-1.8.0-1.fc23.src.rpm
Description: 

eDeploy is a tool to provision and update systems (physical or
virtual) using trees of files instead of packages or VM images.

Fedora Account System Username: flepied

Comment 1 Haïkel Guémar 2015-03-25 21:32:12 UTC
1. doesn't build due to missing BuildRequires: python-sphinx
2. if you use %autosetup, you don't need to use %patch as the former will apply it for you. I suggest using -S git option as it allows you to manage your patches with git (the standard for RDO btw)
3.use %{_sharedstatedir} instead of /var/lib
4. according guidelines, you shouldn't deploy web applications into /var/www but %{_datadir}/%{name}
5. sources shouldn't be put in /usr/src (non-standard), are they required ? If they're not, I'd suggest to drop them.
6.missing Requires: python-netaddr, python-pexpect

Comment 2 Frédéric Lepied 2015-03-26 02:01:35 UTC
(In reply to Haïkel Guémar from comment #1)
> 1. doesn't build due to missing BuildRequires: python-sphinx
> 2. if you use %autosetup, you don't need to use %patch as the former will
> apply it for you. I suggest using -S git option as it allows you to manage
> your patches with git (the standard for RDO btw)
> 3.use %{_sharedstatedir} instead of /var/lib
> 4. according guidelines, you shouldn't deploy web applications into /var/www
> but %{_datadir}/%{name}
> 5. sources shouldn't be put in /usr/src (non-standard), are they required ?
> If they're not, I'd suggest to drop them.
> 6.missing Requires: python-netaddr, python-pexpect

Thanks for the review, Haïkel ! I adapted the packaging according to your comments. Could you take a look again ?

Thanks,
Fred

Comment 3 Frédéric Lepied 2015-04-15 19:43:37 UTC
Updated to version 1.9.0:

Spec URL: https://flepied.fedorapeople.org/edeploy.spec
SRPM URL: https://flepied.fedorapeople.org/edeploy-1.9.0-1.fc23.src.rpm

Comment 4 Frédéric Lepied 2015-05-07 21:41:27 UTC
Updated to version 1.10.0:

Spec URL: https://flepied.fedorapeople.org/edeploy.spec
SRPM URL: https://flepied.fedorapeople.org/edeploy-1.10.0-1.fc23.src.rpm

Comment 5 Frédéric Lepied 2015-05-19 14:10:23 UTC
Updated to version 1.11.0:

Spec URL: https://flepied.fedorapeople.org/edeploy.spec
SRPM URL: https://flepied.fedorapeople.org/edeploy-1.11.0-1.fc23.src.rpm

Comment 6 Michael S. 2015-05-19 19:22:35 UTC
So, a few notes:

1) %{_sharedstatedir}/edeploy/ seems to be unowned

2) I prefer to have 1 requires on each line ( easier to see with diff, but just a personal preference, given for the pleasure of being pedantic as I promised to be )

3) no need for BuildArch: noarch on the -devel package, that's redundant with the main rpm.

4) there is no file who details the license in a %license, please add it.


5) %{_datadir}/%{name} is unowned

6) %attr(0644, apache, apache) %{_sharedstatedir}/edeploy/*.cmdb

this would requires a user apache on the system, and it is created by another rpm, which in turn should be required, if I am not wrong. 

I also suspect this might requires some selinux policy change, but that's outside of this review and package. 

In fact, if the files can be modified by apache, they should be marked as %ghost or something, and if they can't, then they shouldn't belong to apache, no ?

Could you clarify that part ?

7) any reason to not run the tests in %check ( since there is tox.ini ) ? 
if so, I think we should have a comment in the rpm saying why they are not enabled, and if we can run, then run them. 

I might have others stuff to see once I look more on the policy for web application, and dig a bit more in the source code.

Comment 7 Frédéric Lepied 2015-05-20 20:18:37 UTC
I created a version version 1.11.0-2 to address your concerns:

Spec URL: https://flepied.fedorapeople.org/edeploy.spec
SRPM URL: https://flepied.fedorapeople.org/edeploy-1.11.0-2.fc23.src.rpm

The only point I haven't addressed is 7 as I cannot add a %check section because bash8 is not packaged and is used by the tests.

Comment 8 Haïkel Guémar 2015-05-23 22:10:09 UTC
1. Drop PreReq tag, guidelines advise against it
2. Drop Group tags, it's not used anymore
3. Doc is large enough to be in its own subpackage
4. BR: python2-devel
5. you enable %check as I submitted python-bash8 (as enabling tests in packages builds will be a goal for Liberty, I will have to submit it anyway)
https://bugzilla.redhat.com/show_bug.cgi?id=1224513

Comment 9 Frédéric Lepied 2015-05-24 21:36:21 UTC
I created a version 1.11.0-4 to address your concerns:

Spec URL: https://flepied.fedorapeople.org/edeploy.spec
SRPM URL: https://flepied.fedorapeople.org/edeploy-1.11.0-4.fc23.src.rpm

Please don't block on non available package. I have added a %check section and I'll activate the bash8 script later when bash8 will be available.

Comment 10 Upstream Release Monitoring 2015-12-06 18:26:12 UTC
pbrobinson's scratch build of linux-user-chroot?#b7afe5173cbd31b029b027b6f8a14baa5e6ce87a for epel7-archbootstrap and git://pkgs.fedoraproject.org/linux-user-chroot?#b7afe5173cbd31b029b027b6f8a14baa5e6ce87a failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12089939


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