Bug 837313 - Review Request: gssproxy - A proxy for GSSAPI credential handling
Review Request: gssproxy - A proxy for GSSAPI credential handling
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Guenther Deschner
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-03 08:39 EDT by Guenther Deschner
Modified: 2015-02-18 09:55 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-02-18 09:55:00 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
asn: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Guenther Deschner 2012-07-03 08:39:49 EDT
Spec URL: http://fedorapeople.org/~gd/gssproxy/gssproxy.spec
SRPM URL: http://fedorapeople.org/~gd/gssproxy/gssproxy-0.0.1-2.fc17.src.rpm
Description: gssproxy proxies the GSSAPI communication on a local machine to
allow better communication with GSSAPI from the kernel and to allow fine grained
control over applications access to cryptographic key material stored in
keytabs.
Fedora Account System Username: gd
Comment 1 Andreas Schneider 2012-07-03 11:36:47 EDT
I've reviewed the spec and the source RPM. All good :)
Comment 2 Guenther Deschner 2012-07-11 10:27:43 EDT
New Package SCM Request
=======================
Package Name: gssproxy
Short Description: A proxy for GSSAPI credential handling
Owners: gd simo
Branches: f18 devel
InitialCC:
Comment 3 Toshio Ernie Kuratomi 2012-07-12 12:18:54 EDT
Andreas -- Could you go back and please do a proper Fedora Review?  Just looking at the spec file and not attempting to build or understand gssproxy at all, I immediately see two things wrong with it (Not a complete URL to the Source, scriptlets for installing systemd units are wrong).  The lack of any Requires: is also fishy to me... I'm thinking there's probably at least a missing Requires: on some systemd package.  The ldconfig invocation in the %post script is also fishy -- I don't see any libraries being installed and if there were libraries installed, there's no matching ldconfig call in %postun.  If you dig into the package you may find even more stuff wrong.

For help with doing a proper review, you can look at: http://fedoraproject.org/wiki/Packaging:ReviewGuidelines

and 

http://fedoraproject.org/wiki/Packaging:Guidelines

The scriptlets for systemd are on:

http://fedoraproject.org/wiki/Packaging:Systemd
Comment 4 Toshio Ernie Kuratomi 2012-07-12 12:27:43 EDT
Actually, that was the general systemd-in-packaging page.  The scriptlets themselves are on:

http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd
Comment 5 Guenther Deschner 2012-07-13 06:53:26 EDT
Ok, added various fixes for systemd/rpmlint issues, etc. Also uses first real upstream tarball 0.0.2 now. Please re-review.

Spec URL: http://fedorapeople.org/~gd/gssproxy/gssproxy.spec
SRPM URL: http://fedorapeople.org/~gd/gssproxy/gssproxy-0.0.2-3.fc17.src.rpm
Comment 6 Andreas Schneider 2012-07-16 07:15:50 EDT
The fixes look fine for me and it builds and installs just fine too.
Comment 7 Gwyn Ciesla 2012-07-16 11:06:27 EDT
Git done (by process-git-requests).

No need to request f18, currently ==devel.
Comment 8 Toshio Ernie Kuratomi 2012-07-16 12:03:41 EDT
This package still doesn't look like it has been properly reviewed.  Andreas, there's many templates out there of what needs to be checked on review that you could use (for instance: https://fedoraproject.org/wiki/User:Tibbs/Review_Template ).  You can also just use http://fedoraproject.org/wiki/Packaging:ReviewGuidelines and post that in checklist form.  If you aren't checking at least the things mentioned in the Packaging Guidelines, then you aren't doing a proper review.  So far, your reviews don't appear to have done that (as you missed easily spotted things after your initial review in Comment #1 and your followup in Comment #6 doesn't say that you then checked the package against the Guidelines, only that you built it and installed it.)

If you are doing the full review just not writing it down, also remember that the package review is a means of communication.  If you don't write down what has been checked, what was good, and what was bad, you aren't recording information on the package that people may need later when figuring out why or when a package started doing something wrong.

Guenther, please don't build this package until Andreas has done a full review -- I see at least one other thing that Andreas hasn't done which should lead to either a change in %files or a comment about why it is the way it is.
Comment 9 Guenther Deschner 2012-07-16 13:00:27 EDT
Sorry, package already built. 

Toshio, what in particular should I fix (and Andreas review) ? You mentioned an item in the files section.
Comment 10 Toshio Ernie Kuratomi 2012-07-16 13:58:53 EDT
Andreas needs to do the review.  If I keep telling what things I find, at some point I've done the review, not Andreas.  For you, and the package, that's fine.  But for Andreas, the only thing that could be done then is decide he doesn't know how to be a Fedora Contributor and ask FESCo to put him on probation.  (Which FESCo might or might not grant -- I really hate to go down that path so I try to get people on the right path before it gets to that state :-)
Comment 11 Andreas Schneider 2012-07-17 05:12:12 EDT
--- gssproxy.spec.orig  2012-07-13 12:46:58.000000000 +0200
+++ gssproxy.spec       2012-07-16 21:33:02.740748184 +0200
@@ -36,7 +36,7 @@
 
 
 %description
-A proxy for GSSAPI credential handling
+A proxy for GSSAPI credential handling.
 
 %prep
 %setup -q
@@ -67,13 +67,14 @@
 
 %files
 %defattr(-,root,root,-)
+%doc COPYING
 %{_unitdir}/gssproxy.service
 %{_sbindir}/gssproxy
 %dir %{gsspstatedir}
 %attr(755,root,root) %dir %{pipepath}
 %attr(755,root,root) %dir %{pubconfpath}
-%attr(700,root,root) %dir /%{_sysconfdir}/gssproxy
-%attr(0600,root,root) %config(noreplace) /%{_sysconfdir}/gssproxy/gssproxy.conf
+%attr(700,root,root) %dir %{_sysconfdir}/gssproxy
+%attr(0600,root,root) %config(noreplace) %{_sysconfdir}/gssproxy/gssproxy.conf
 %{_mandir}/man5/gssproxy.conf.5*
 %{_mandir}/man8/gssproxy.8*

For a full review with every step documented you have to wait till tomorrow.
Comment 12 Guenther Deschner 2012-07-18 07:11:42 EDT
Ok, added these and more fixes to the specfile.
Comment 13 Guenther Deschner 2015-02-18 09:55:00 EST
gssproxy is in fedora meanwhile.

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