Bug 837313

Summary: Review Request: gssproxy - A proxy for GSSAPI credential handling
Product: [Fedora] Fedora Reporter: Guenther Deschner <gdeschner>
Component: Package ReviewAssignee: Guenther Deschner <gdeschner>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: a.badger, asn, package-review, pingou, ssorce
Target Milestone: ---Flags: asn: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-02-18 14:55:00 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:

Description Guenther Deschner 2012-07-03 12:39:49 UTC
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 15:36:47 UTC
I've reviewed the spec and the source RPM. All good :)

Comment 2 Guenther Deschner 2012-07-11 14:27:43 UTC
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 16:18:54 UTC
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 16:27:43 UTC
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 10:53:26 UTC
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 11:15:50 UTC
The fixes look fine for me and it builds and installs just fine too.

Comment 7 Gwyn Ciesla 2012-07-16 15:06:27 UTC
Git done (by process-git-requests).

No need to request f18, currently ==devel.

Comment 8 Toshio Ernie Kuratomi 2012-07-16 16:03:41 UTC
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 17:00:27 UTC
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 17:58:53 UTC
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 09:12:12 UTC
--- 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 11:11:42 UTC
Ok, added these and more fixes to the specfile.

Comment 13 Guenther Deschner 2015-02-18 14:55:00 UTC
gssproxy is in fedora meanwhile.