Bug 1059281

So :
- the list on Requires on 1 single line is a bit too long for my taste, 1 requires per line would likely be better in diff, etc. And it would also be better to have precise requires ( ie, on the specific version )

- the requires on the -log subpackage :
Requires: /bin/egrep

any reason to not use %{_bindir}/ like the rest ?

- description could be enhanced to say this provides more than plugin for nagios 

- %{_libdir}/monitoring/ is unowned

- the various setuid plugin are not a good ideas IMHO, not sure if that requires FESCO approval or something. I would feel better if this was using capacity.

- I see a few tests for the plugins and the lib, yet no %check, is this normal ?

- there is a few tarballs of perl source code, but this is not used for now, just mentioning for documenting it

- why do all plugin provides monitoring-plugins ? ( and not a requires ? )

(In reply to Michael Scherer from comment #1)
> So :
> - the list on Requires on 1 single line is a bit too long for my taste, 1
> requires per line would likely be better in diff, etc. And it would also be
> better to have precise requires ( ie, on the specific version )
> 
> - the requires on the -log subpackage :
> Requires: /bin/egrep
> 
> any reason to not use %{_bindir}/ like the rest ?

Done.

> 
> - description could be enhanced to say this provides more than plugin for
> nagios 

Fixed.

> 
> - %{_libdir}/monitoring/ is unowned

Fixed.

> 
> - the various setuid plugin are not a good ideas IMHO, not sure if that
> requires FESCO approval or something. I would feel better if this was using
> capacity.

I can start working on that upstream. There are lots of other packages that use setuid, though; does this block the review?

> 
> - I see a few tests for the plugins and the lib, yet no %check, is this
> normal ?

The tests require access to different services, like an http server or an ntp peer; they'll never pass inside a buildroot.

> 
> - there is a few tarballs of perl source code, but this is not used for now,
> just mentioning for documenting it
> 
> - why do all plugin provides monitoring-plugins ? ( and not a requires ? )

Fixed.

I've uploaded a new srpm here: http://skottler.fedorapeople.org/monitoring-plugins-1.5-4.fc21.src.rpm. The spec is also updated, but in the same location.

I am gonna ask for setuid, since we had a feature for that ( https://fedoraproject.org/wiki/Features/RemoveSETUID ). I do not think this would block the review, but I prefer to be sure. And to be honest, i do not see that as being hard to fix. Most of them just need the capability for raw socket.

For the tests, if they need something, ok, this would be hard to do.

I will update the review later.

from #fedora-devel

16:55:46|  misc> is setuid packages required to be checked by FESCO or something ( 
                 https://bugzilla.redhat.com/show_bug.cgi?id=1059281 ) ?
16:55:56|  misc> ( as we try to be setuid-free )

16:59:39|  sgallagh> misc: Yes, any setuid root packages have to be approved.
17:00:49|  sgallagh> (of course, setuid non-root is acceptable, since it's non-privileged)

so Fesco time ?

@misc, what's the way forward with this? I've already gotten a _hardened_build and sgallagher wasn't sure about whether or not there's a document about setuid exceptions.

I would ask to Fesco to be sure, but i will in the mean time still continue the review.

Created attachment 915872 [details]
Comment

(This comment was longer than 65,535 characters and has been moved to an attachment by Red Hat Bugzilla).

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.

Closing this request now, as reporter's Bugzilla account has been disabled.