Bug 909471

Summary: Review Request: rubygem-openshift-origin-auth-remote-user - OpenShift plugin for remote-user authentication
Product: [Fedora] Fedora Reporter: Troy Dawson <tdawson>
Component: Package ReviewAssignee: Michael Scherer <misc>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: misc, notting, package-review, vondruch
Target Milestone: ---Flags: misc: fedora‑review+
kevin: 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: 2013-04-11 06:00:38 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Troy Dawson 2013-02-08 16:04:46 EST
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-remote-user.spec
SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-remote-user-1.4.3-1.fc18.src.rpm
Description: Provides a remote-user auth service based plugin for OpenShift
Fedora Account System Username: tdawson
Comment 1 Troy Dawson 2013-02-08 16:09:14 EST
rpmlint output:
rpmlint rubygem-openshift-origin-auth-remote-user.spec rubygem-openshift-origin-auth-remote-user-1.4.3-1.fc18.src.rpm rubygem-openshift-origin-auth-remote-user-1.4.3-1.fc18.noarch.rpm rubygem-openshift-origin-auth-remote-user-doc-1.4.3-1.fc18.noarch.rpm
rubygem-openshift-origin-auth-remote-user-doc.noarch: W: non-conffile-in-etc /etc/openshift/plugins.d/openshift-origin-auth-remote-user.conf.example
3 packages and 1 specfiles checked; 0 errors, 1 warnings.

- Upstream doesn't want to mark that file as a conffile, because it is an example file.
Comment 2 Michael Scherer 2013-02-25 11:13:06 EST
Why is there a favicon in source2 ? ( since that's used only for test ? )
Comment 3 Troy Dawson 2013-02-25 11:37:40 EST
That was to get rid of the rpmlint empty file errors.
Comment 4 Troy Dawson 2013-03-21 17:36:40 EDT
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-remote-user.spec
SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-remote-user-1.5.6-2.fc18.src.rpm

- Updated to the latest stable release
- Updated spec file to work with new F19+ requirements.
Comment 5 Michael Scherer 2013-03-21 20:08:26 EDT
Why is there a %post to change the configuration of a file that is not in the package ?

%post
if [ $1 -ne 1 ] # this is an update; fix the previously configured realm.
then
  sed -i -e 's/AuthName.*/AuthName "OpenShift Broker API"/' /var/www/openshift/broker/httpd/conf.d/openshift-origin-auth-remote-user.conf
fi


Package seems to be not installable on rawhide at the moment due to ruby 2.0 ( but I need to investigate )
Comment 6 Vít Ondruch 2013-03-27 05:00:16 EDT
Just a few points, mostly without explanation, since they are the same as in bug 894524 :)

* %{_docdir}? what is that? shouldn't it be %{gem_docdir}?

* exclude %{gem_cache}

* %gem_install macro

* vendor?

* %{gem_libdir} instead of %{gem_instdir}/lib

* %{gem_docdir} and %doc marking.
Comment 7 Troy Dawson 2013-03-27 15:16:46 EDT
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-remote-user.spec
SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-remote-user-1.5.6-3.fc18.src.rpm

- %post script - removed.  This was for an old conf format used back when this was first started.
- %{_docdir} docs moved to %{gem_docdir}.
- exclude %{gem_cache} - Done
- %gem_install macro - Used
- vendor - You must have gotten this mixed up with the other openshift rubygem.  I couldn't find any directory and/or file with that name in it.
- %{gem_libdir} instead of %{gem_instdir}/lib - Done
- %{gem_docdir} and %doc marking - again, I think you have this mixed up with the other rubygem spec file.

* I will work on upstream to get these fixed upstream.
* Builds and installs on rawhide (F20)
Comment 8 Vít Ondruch 2013-03-27 17:05:19 EDT
(In reply to comment #7)
> - %{_docdir} docs moved to %{gem_docdir}.

Ups, now I read the .spec file more carefully and get the original idea with the %{_docdir} and I must say that it was correct idea. Little explanation would be enough, since I was confused :) However the approach was a bit clumsy. It should be enough to do:

%install
install -m 644 %{SOURCE1} .

%files
%doc %{name}-doc.README

I apologize for confusion :/

> - vendor - You must have gotten this mixed up with the other openshift
> rubygem.  I couldn't find any directory and/or file with that name in it.
> - %{gem_docdir} and %doc marking - again, I think you have this mixed up
> with the other rubygem spec file.

Sorry, it might be. It was just quick fly through :)

I found another nit: you should use %{_sysconfdir} instead of /etc
Comment 9 Vít Ondruch 2013-03-27 18:38:48 EDT
And I forgot to ask you about test suite ...
Comment 10 Troy Dawson 2013-03-28 15:40:52 EDT
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-remote-user.spec
SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-remote-user-1.5.6-4.fc18.src.rpm

Installed the doc.README as you suggested.

I looked into the test suite.  It requires part of the openshift enviroement needs to be setup for it to work.  In particular it needs a mongodb setup.
Doesn't look like the test suite is do-able in an rpm enviroment.
Comment 11 Michael Scherer 2013-03-28 20:52:47 EDT
Technically, it could be done ( after all, if we can make a X server work ... ), but maybe that's too complex. No way to run only part of the test ?
Comment 12 Troy Dawson 2013-04-01 09:51:35 EDT
OK, at this point I'm going to have to say "really?"
I have to wait months at a time in between comments.  The ruby build requirements have changed twice while waiting, so I have to repackage according to the new ruby build requirements.  (Though I have to say the new macro's make things much cleaner.)  The package has updated twice during that time.
Testing is an optional thing for ruby packages (at least last time I checked).
The purpose of this package/plugin is to do authentication for openshift users.  Those users are supposed to be in an openshift mongodb database.
Can I write up something that installs a mongodb database, adds some test openshift users in it, then test against that.  I suppose I could.
If I had some assurance that that is where it stops, maybe I'd try.  But it's really starting to feel more like your looking for reasons not to pass my package than to help me get the package right.
Comment 13 Troy Dawson 2013-04-01 10:06:46 EDT
My apologies.  I shouldn't have said that.  I'm in a bit of a time crunch for this package and it's starting to show.

As I said above, the purpose of this package/plugin is to do authentication for openshift users.  Those users are supposed to be in an openshift mongodb database.

Could I write some type of setup for doing this, possibly.  I'm not a database guy, but I could work on it.  But it would take alot more time that I feel I have.  

I would really like this package because I use it in my tests for the OpenShift Origin feature testing in F19.  Time is currently short on that, so I'm feeling a bit under pressure for this package.

If we could get this package reviewed without the tests I would appreciate it.

Thank You
Comment 14 Michael Scherer 2013-04-01 11:45:15 EDT
For sure, we can get without the tests, but given the changing nature of ruby, I strongly recommend to have them enabled later and  try to spend sometime on having a way to make them work. I will see if I can provide a patch for that. 

Anyway, I am not up to date regarding the changes to the ruby packaging policy, but there is a item in fedora review for : 
"Gem package should exclude cached Gem."

So can you exclude the cache from the %files list ?

https://fedoraproject.org/wiki/Packaging_talk:Ruby#Cached_.gem_file

Another issue is the requires. The gemspec talk of 

The spec say :
Requires:      %{?scl:%scl_prefix}rubygems
Requires:      %{?scl:%scl_prefix}rubygem(json)
Requires:      %{?scl:%scl_prefix}rubygem(rails)
Requires:      rubygem(openshift-origin-common)
Requires:      rubygem(openshift-origin-controller)
Requires:      openshift-origin-broker

Gemspec only speak of :
  s.add_dependency('openshift-origin-controller')
  s.add_dependency('json')

After looking at the code, there is no need for the json module ( except for test ).
It seems I forgot to make PR for that after seeing 6 months ago :
https://github.com/mscherer/origin-server/commit/982375a196d11f887097c751189e032ccdfa9700 


Anyway, this package should be good, provided thse 2 minors changes are fixed so approved. 

Package Review
==============

Key:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


[!]: Gem package should exclude cached Gem.
[!]: No need to requires ruby json

===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package requires other packages for directories it uses.
     Note: No known owner of /var/www/openshift
[x]: Package must own all directories that it creates.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf %{buildroot} present but not required
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Fully versioned dependency in subpackages, if present.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in rubygem-
     openshift-origin-auth-remote-user-doc
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

Ruby:
[x]: Platform dependent files must all go under %{gem_extdir}, platform
     independent under %{gem_dir}.
[x]: Gem package must not define a non-gem subpackage
[x]: Gem package is named rubygem-%{gem_name}
[x]: Package contains BuildRequires: rubygems-devel.
[x]: Gem package must define %{gem_name} macro.
[x]: Pure Ruby package must be built as noarch
[x]: Package contains Requires: ruby(abi).

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[-]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

Ruby:
[!]: Gem package should exclude cached Gem.
[!]: Test suite of the library should be run.
[x]: Specfile should use macros from rubygem-devel package.
     Note: The specfile doesn't use these macros: %{gem_libdir}, %exclude
     %{gem_cache}

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: rubygem-openshift-origin-auth-remote-user-1.5.6-2.fc18.noarch.rpm
          rubygem-openshift-origin-auth-remote-user-doc-1.5.6-2.fc18.noarch.rpm
rubygem-openshift-origin-auth-remote-user-doc.noarch: W: non-conffile-in-etc /etc/openshift/plugins.d/openshift-origin-auth-remote-user.conf.example
2 packages and 0 specfiles checked; 0 errors, 1 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint rubygem-openshift-origin-auth-remote-user rubygem-openshift-origin-auth-remote-user-doc
rubygem-openshift-origin-auth-remote-user-doc.noarch: W: non-conffile-in-etc /etc/openshift/plugins.d/openshift-origin-auth-remote-user.conf.example
2 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'



Requires
--------
rubygem-openshift-origin-auth-remote-user (rpmlib, GLIBC filtered):
    /bin/sh
    openshift-origin-broker
    ruby(abi)
    rubygem(json)
    rubygem(openshift-origin-common)
    rubygem(openshift-origin-controller)
    rubygem(rails)
    rubygems

rubygem-openshift-origin-auth-remote-user-doc (rpmlib, GLIBC filtered):
    /usr/bin/env



Provides
--------
rubygem-openshift-origin-auth-remote-user:
    rubygem(openshift-origin-auth-remote-user)
    rubygem-openshift-origin-auth-remote-user

rubygem-openshift-origin-auth-remote-user-doc:
    rubygem-openshift-origin-auth-remote-user-doc



MD5-sum check
-------------
http://mirror.openshift.com/pub/openshift-origin/source/rubygem-openshift-origin-auth-remote-user/openshift-origin-auth-remote-user-1.5.6.gem :
  CHECKSUM(SHA256) this package     : 83f8017b9dcd79811680c7b3d3dbc45fed32e1646cbf93a198ff1d384d7883ea
  CHECKSUM(SHA256) upstream package : 83f8017b9dcd79811680c7b3d3dbc45fed32e1646cbf93a198ff1d384d7883ea


Generated by fedora-review 0.4.0 (cf29f98) last change: 2013-02-08
Buildroot used: fedora-18-x86_64
Command line :./try-fedora-review -b 909471
Comment 16 Michael Scherer 2013-04-01 14:09:22 EDT
Mh, I tought I had set fedora-review to + already, but it was not.
Comment 17 Troy Dawson 2013-04-01 15:01:20 EDT
New Package SCM Request
=======================
Package Name: rubygem-openshift-origin-auth-remote-user
Short Description: OpenShift plugin for remote-user authentication
Owners: tdawson maxamillion
Branches: f17 f18 f19
InitialCC:
Comment 18 Kevin Fenzi 2013-04-01 15:42:11 EDT
Git done (by process-git-requests).
Comment 19 Fedora Update System 2013-04-01 17:03:15 EDT
rubygem-openshift-origin-auth-remote-user-1.5.6-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/rubygem-openshift-origin-auth-remote-user-1.5.6-5.fc18
Comment 20 Vít Ondruch 2013-04-02 02:52:36 EDT
(In reply to comment #12)
> OK, at this point I'm going to have to say "really?"

Troy, although it may be annoying, I strongly encourage everybody to include test suite, even some basic one. It helps a lot during times like this, i.e. new Ruby is released and there is done automatic rebuild, or even during mass rebuild of Fedora, since your package may become failing anytime, due to changed dependencies, etc. So it is safe net for every Fedora user.

I as a maintainer am glad, if the test suite fails during such occasion and I am happy to fix any issue it uncovers. Naturally, I am happier if I see that although there were changes, the test suite still passes.

Also, please note, I am not sure how much are you involved in upstream development of this package, but this is upstream question IMO, so there should be no hard feeling. It should be no blocker, but I have to ask.

> I have to wait months at a time in between comments.

Sorry for that, but you can always ping me.
Comment 21 Fedora Update System 2013-04-03 00:34:43 EDT
rubygem-openshift-origin-auth-remote-user-1.5.6-5.fc18 has been pushed to the Fedora 18 testing repository.
Comment 22 Fedora Update System 2013-04-11 06:00:41 EDT
rubygem-openshift-origin-auth-remote-user-1.5.6-5.fc18 has been pushed to the Fedora 18 stable repository.