This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours

Bug 845021

Summary: Review Request: rubygem-openshift-origin-auth-mongo - OpenShift plugin for mongo authentication service
Product: [Fedora] Fedora Reporter: Troy Dawson <tdawson>
Component: Package ReviewAssignee: Tom "spot" Callaway <tcallawa>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: admiller, maxamillion, misc, notting, package-review, tcallawa, vondruch
Target Milestone: ---Flags: tcallawa: fedora‑review+
limburgher: 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: 2012-09-17 18:03:59 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On: 760696, 839064, 844013    
Bug Blocks:    

Description Troy Dawson 2012-08-01 09:51:44 EDT
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-mongo.spec
SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-mongo-0.8.5-4.fc18.src.rpm
Description: Provides a mongo authentication service based plugin

Fedora Account System Username: tdawson

Notes:
This package was originally rubygem-swingshift-mongo-plugin
https://bugzilla.redhat.com/show_bug.cgi?id=842890
The package has been renamed to rubygem-openshift-origin-auth-mongo
Since the last comments in that review I have done the following
- Changed the package and gem names.
- Removed unessesary

Note: Because only the package and gemname have been changed, there are some libraries and paths that still have the old name.  This is known and shouldn't affect functionality.
Comment 1 Troy Dawson 2012-08-01 09:57:46 EDT
*** Bug 842890 has been marked as a duplicate of this bug. ***
Comment 2 Michael Scherer 2012-08-01 17:25:00 EDT
Shouldn't it requires rubygem(mongo), it that use mongodb as a backend ?
Comment 3 Troy Dawson 2012-08-01 17:47:52 EDT
Usually developers use bson or bson_ext when accessing mongodb via ruby.  So I would suspect it would want rubygem(bson)
But doing greps through the code don't show any reference to bson.  The only references to mongo are internal references in the code, mainly because the pathname has mongo in it.
So, from what I can see, no, it doesn't need rubygem(mongo) or rubygem(bson).
Comment 4 Michael Scherer 2012-08-02 01:39:20 EDT
The gemspec say and the code say that :
- rails is required ( guess this is already pull somewhere in the stack ), not sure this is pulled already in the stack ( in fact, due to the renaming going on, I have some trouble to follow what need what, so maybe the first thing would be to sort the bugs deps )

- rubugem(activeresource), for ss-create-user, not pulled by rubygem(openshift-origin-common)


lib/swingshift-mongo-plugin/lib/swingshift/mongo_auth_service.rb
this seems to use mongodb ruby binding 
( http://api.mongodb.org/ruby/current/Mongo/Connection.html )
I didn't found the needed package on Fedora 17 with yum.

This would be pulled by rubygem(openshift-origin-common), but I am not sure if this belong really here ( cf https://bugzilla.redhat.com/show_bug.cgi?id=839064#c13 )



Also, some test seems to exist, are they usable ?
Comment 5 Troy Dawson 2012-08-02 11:25:40 EDT
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-mongo.spec
SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-mongo-0.8.5-5.fc18.src.rpm

- Added Requires: rubygem(mongo)
- Added Requires: rubygem(activeresource)

Notes for tests:
- Currently no.  The tests do not work in a mock/koji enviroment.
Comment 6 Troy Dawson 2012-08-02 18:48:54 EDT
I would like to add FAS account name maxamillion to this review as I'll be out of town for the next week and don't want to be a blocker.

Fedora Account System Username: maxamillion tdawson
Comment 7 Tom "spot" Callaway 2012-08-07 11:31:14 EDT
Does this need the non rubygem subpackage? The guidelines state that this is no longer needed:

https://fedoraproject.org/wiki/Packaging/Ruby#Packaging_for_Gem_and_non-Gem_use

There is also this from rpmlint:

rubygem-openshift-origin-auth-mongo.noarch: W: wrong-file-end-of-line-encoding /usr/share/gems/doc/openshift-origin-auth-mongo-0.8.5/ri/cache.ri

Should be easy to fix (sed -i 's/\r//' $FILE).

rubygem-openshift-origin-auth-mongo.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-auth-mongo-0.8.5/ri/Swingshift/MongoAuthService/user_exists%3f-i.ri %3f

I _think_ this is a false positive, because the file name contains "%3f", but you should check to ensure that filename is correct, it seems odd.

Everything else seems okay here, resolve these issues and I will finish this review.
Comment 8 Adam Miller 2012-08-07 16:05:15 EDT
SPEC URL: http://maxamillion.fedorapeople.org/rubygem-openshift-origin-auth-mongo.spec
SRPM URL: http://maxamillion.fedorapeople.org/rubygem-openshift-origin-auth-mongo-0.8.5-6.fc17.src.rpm

I got rid of the non rubygem package as its not needed.

The wrong-file-end-of-line-encoding persisted through my attempts with sed and dos2unix, I'm open to suggestion but I'm not familiar with another method of solving that one.

I believe the "%3f" is a false positive, that is apparently a common issue with ruby packages.
Comment 9 Vít Ondruch 2012-08-09 07:12:45 EDT
(In reply to comment #8)
> The wrong-file-end-of-line-encoding persisted through my attempts with sed
> and dos2unix, I'm open to suggestion but I'm not familiar with another
> method of solving that one.

Please don't do that, That is false positive. That is binary file no matter how it looks. It is written using Ruby's Marshal.dump, so you have effectively corrupted the file.
Comment 10 Tom "spot" Callaway 2012-08-09 09:16:36 EDT
I learn something new everyday! This probably is good information for the Ruby packaging Guidelines.
Comment 11 Tom "spot" Callaway 2012-08-09 09:30:40 EDT
Adam, you need to undo the end-of-line encoding "fix", and you don't want to create the symlinks that used to be in the non-rubygem package.

I think I can finish this off with those two items fixed.
Comment 12 Adam Miller 2012-08-09 09:55:26 EDT
SPEC URL: http://maxamillion.fedorapeople.org/rubygem-openshift-origin-auth-mongo.spec
SRPM URL: http://maxamillion.fedorapeople.org/rubygem-openshift-origin-auth-mongo-0.8.5-7.fc17.src.rpm

I learned something new as well! :) ... I'm still new to ruby packaging so its been fun. I've removed the attempt to fix the false positive.
Comment 13 Adam Miller 2012-08-09 10:03:59 EDT
SPEC URL: http://maxamillion.fedorapeople.org/rubygem-openshift-origin-auth-mongo.spec
SRPM URL: http://maxamillion.fedorapeople.org/rubygem-openshift-origin-auth-mongo-0.8.5-8.fc17.src.rpm


Grrr... I missed the mid-air collision. Now without crufty symlinks to the old ruby_vendorlibdir.
Comment 14 Vít Ondruch 2012-08-09 10:18:37 EDT
(In reply to comment #10)
> I learn something new everyday! This probably is good information for the
> Ruby packaging Guidelines.

Better to fix rpmlint (if possible), but I did not find a time yet :/
Comment 15 Vít Ondruch 2012-08-09 10:39:56 EDT
* Missing dependency on rubygem(openshift-origin-controller)?
  - This dependency is specified in .gemspec, so it should be reflected also in
    the .spec

* Documentation in -doc subpackage
  - Have you considered to put the documentation into -doc subpackage? It is
    typically substantial part of the rubygem- package payload.

* rubygem(mongo)
  - The .gemspec does not require it nor the code, so why is it there? I saw
    there was some confusion about it but it should be probably pulled in by
    rubygem(openshift-origin-controller)?
Comment 16 Adam Miller 2012-08-09 11:40:35 EDT
SPEC URL: http://maxamillion.fedorapeople.org/rubygem-openshift-origin-auth-mongo.spec
SRPM URL: http://maxamillion.fedorapeople.org/rubygem-openshift-origin-auth-mongo-0.8.5-9.fc17.src.rpm

I must have overlooked the openshift-origin-controller dep in the transition between Troy and myself on the package hand off, fixed.

Documentation subpackage created and rubygem-mongo dep removed.
Comment 17 Tom "spot" Callaway 2012-08-10 16:03:57 EDT
= REVIEW =

- rpmlint returns:
rubygem-openshift-origin-auth-mongo.noarch: W: no-manual-page-for-binary ss-register-user
rubygem-openshift-origin-auth-mongo-doc.noarch: W: spelling-error Summary(en_US) mongodb -> mongoloid
rubygem-openshift-origin-auth-mongo-doc.noarch: W: spelling-error %description -l en_US mongodb -> mongoloid
rubygem-openshift-origin-auth-mongo-doc.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-auth-mongo-0.8.5/ri/Swingshift/MongoAuthService/user_exists%3f-i.ri %3f
rubygem-openshift-origin-auth-mongo-doc.noarch: W: wrong-file-end-of-line-encoding /usr/share/gems/doc/openshift-origin-auth-mongo-0.8.5/ri/cache.ri
3 packages and 0 specfiles checked; 0 errors, 5 warnings.

All safe to ignore for ruby packages.

- package meets naming guidelines
- package meets packaging guidelines
- license (ASL 2.0) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream (ebb38a4da9a2edeec5407b0b7492bc7d336b73f3021ae9c854abf65ad611ef2f)
- package compiles on devel (noarch)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- doc subpackage OK
- nothing in %doc affects runtime
- no need for .desktop file

APPROVED.
Comment 18 Adam Miller 2012-08-10 17:00:53 EDT
New Package SCM Request
=======================
Package Name: rubygem-openshift-origin-auth-mongo
Short Description: OpenShift plugin for mongo authentication service
Owners: maxamillion tdawson
Branches: f17 f18 el6
InitialCC:
Comment 19 Jon Ciesla 2012-08-11 11:49:40 EDT
Git done (by process-git-requests).
Comment 20 Vít Ondruch 2012-08-13 01:56:44 EDT
(In reply to comment #18)
> Branches: f17 f18 el6

Since rubygem-mongo goes just into F18+ (which is right IMO), the F17 branch will not be needed I guess. The same for EPEL6.
Comment 21 Fedora Update System 2012-08-22 14:56:07 EDT
rubygem-openshift-origin-auth-mongo-0.8.5-10.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/rubygem-openshift-origin-auth-mongo-0.8.5-10.fc18
Comment 22 Fedora Update System 2012-08-23 00:35:22 EDT
Package rubygem-openshift-origin-auth-mongo-0.8.5-10.fc18:
* should fix your issue,
* was pushed to the Fedora 18 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing rubygem-openshift-origin-auth-mongo-0.8.5-10.fc18'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-12542/rubygem-openshift-origin-auth-mongo-0.8.5-10.fc18
then log in and leave karma (feedback).
Comment 23 Fedora Update System 2012-09-17 18:03:59 EDT
rubygem-openshift-origin-auth-mongo-0.8.5-10.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.