Bug 229323 - Review Request: postgresql-pgpoolAdmin - web-based pgpool-II administration
Review Request: postgresql-pgpoolAdmin - web-based pgpool-II administration
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jon Ciesla
Fedora Package Reviews List
:
Depends On: 229321
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-20 05:21 EST by Devrim GUNDUZ
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-17 16:50:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑review+
devrim: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Devrim GUNDUZ 2007-02-20 05:21:57 EST
Spec URL: http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpoolAdmin.spec
SRPM URL: 
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpoolAdmin-1.0.0-5.src.rpm
Description: 
The pgpool Administration Tool is management tool of pgpool-II. It is
possible to monitor, start, stop pgpool and change settings of pgpool-II.
Comment 1 Jon Ciesla 2007-04-27 07:55:10 EDT
rpmlint on SRPM:
W: postgresql-pgpoolAdmin mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 3)
The specfile mixes use of spaces and tabs for indentation, which is a
cosmetic annoyance.  Use either spaces or tabs for indentation, not both.

Should be an easy fix.

on RPM: 
W: postgresql-pgpoolAdmin symlink-should-be-relative
/usr/share/postgresql-pgpoolAdmin/conf/pgmgt.conf.php
/etc/postgresql-pgpoolAdmin/pgmgt.conf.php
Absolute symlinks are problematic eg. when working with chroot environments.

W: postgresql-pgpoolAdmin dangerous-command-in-%post chmod

Instead of chmod in %post, set permissions with an %attr tag on the %files
section.  Much safer.
Comment 2 Jon Ciesla 2007-05-08 13:44:13 EDT
Ping?
Comment 3 Devrim GUNDUZ 2007-06-02 17:28:10 EDT
Pong:)

New spec:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpoolAdmin.spec
New SRPM:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpoolAdmin-1.0.0-6.src.rpm

Removed chmod calls, and updated the pgpool-II package to solve that issue. Also
fixed whitespace issue. rpmlint complains about user apache, but it is ignorable
(per Tibbs). The only issue left is the symlink problem -- I could not
understand that well.

Regards, Devrim 
Comment 4 Jon Ciesla 2007-06-04 12:44:04 EDT
Looking good.

Basically, for the symlink issue, replace:
ln -s %{_sysconfdir}/%{name}/pgmgt.conf.php
%{buildroot}/%{_pgpoolAdmindir}/conf/pgmgt.conf.php

with:
ln -s ../../../../%{_sysconfdir}/%{name}/pgmgt.conf.php
%{buildroot}/%{_pgpoolAdmindir}/conf/pgmgt.conf.php

Good to see you're back.  I was beginning to think we*'d invaded Turkey and the
media was ignoring it, or something.  Wouldn't put it past us, sadly. :)  Just
hit a busy patch?


*I'm USian.
Comment 5 Devrim GUNDUZ 2007-06-04 14:32:36 EDT
Hi,

(In reply to comment #4)
> Looking good.
> 
> Basically, for the symlink issue, replace:
<snip>

Done. Thanks.

> Good to see you're back.  I was beginning to think we*'d invaded Turkey and 
> the media was ignoring it, or something.  Wouldn't put it past us, sadly. :)  

Heh.

> Just hit a busy patch?

No, I was busy with Turkey's biggest "Linux and Open Source Festival" + work issues.

Anyway, I did not bump up spec file version; but updated SRPM:

New spec:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpoolAdmin.spec
New SRPM:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpoolAdmin-1.0.0-6.src.rpm

Is it ok?

Regards, Devrim
Comment 6 Jason Tibbitts 2007-06-05 14:21:32 EDT
Is someone actually reviewing this?  It's still marked as NEW and the
fedora-review flag is unset.
Comment 7 Jon Ciesla 2007-06-05 14:24:19 EDT
Yes.  I believe the expression is D'oh!  Sorry.
Comment 8 Jon Ciesla 2007-06-06 12:01:58 EDT
Extra / in line 43:
ln -s ../../../../%{_sysconfdir}/%{name}/pgmgt.conf.php
%{buildroot}/%{_pgpoolAdmindir}/conf/pgmgt.conf.php

should be

ln -s ../../../..%{_sysconfdir}/%{name}/pgmgt.conf.php
%{buildroot}%{_pgpoolAdmindir}/conf/pgmgt.conf.php

Mock build fails with:
RPM build errors:
    File not found:
/var/tmp/postgresql-pgpoolAdmin-1.0.0-6.fc6-root-mockbuild/etc/httpd/conf.d/postgresql-pgpoolAdmin.conf

Looks like the conditional at line 46 is failing due to a BuildRequires issue. 
Are you sure you don't want that to just install it no matter what?

Comment 9 Devrim GUNDUZ 2007-06-08 18:34:04 EDT
Hi Jon,

(In reply to comment #8)
> Extra / in line 43:
> ln -s ../../../../%{_sysconfdir}/%{name}/pgmgt.conf.php
> %{buildroot}/%{_pgpoolAdmindir}/conf/pgmgt.conf.php
> 
> should be
> 
> ln -s ../../../..%{_sysconfdir}/%{name}/pgmgt.conf.php
> %{buildroot}%{_pgpoolAdmindir}/conf/pgmgt.conf.php

'k , fixed.

> Mock build fails with:
> RPM build errors:
>     File not found:
>
/var/tmp/postgresql-pgpoolAdmin-1.0.0-6.fc6-root-mockbuild/etc/httpd/conf.d/postgresql-pgpoolAdmin.conf
> 
> Looks like the conditional at line 46 is failing due to a BuildRequires issue.

Added httpd as a BuildRequires.

> Are you sure you don't want that to just install it no matter what?

It is required for security reasons + easy configuration for users.

I did not bump up spec file version; but updated SRPM:

New spec:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpoolAdmin.spec
New SRPM:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpoolAdmin-1.0.0-6.src.rpm

Regards, Devrim
Comment 10 Jon Ciesla 2007-06-12 14:12:41 EDT
Those check out, I'll do the formal review soon.
Comment 11 Jon Ciesla 2007-06-15 13:02:30 EDT
rpmlint now clean, save the acceptible error above.
Package and spec names are good.
Meets packaging guidelines.
Not sure what to make of the licensing.  Spec says BSD, license appears BSD-ish.
 Might want to run it by spot but I think it's OK.
Spec is legible American English.
Source md5 matches.
Builds on i386/f7.
BuildRequires are good.
Locales OK.
No libraries, not relocatable.
Owns all created dirs.
No duplicate files or incorrect permissions.
%clean present and correct.
Macros sane.
Code, not content.
No large docs or runtime doc deps.
No headers, .pc or .la.
No subpackages.
Not a gui app.
No known conflicts. (Don't forget to fix postgresql-pgpool-II, BTW :))
Install starts by clearing buildroot.
All filenames UTF8.


All MUSTs met.

APPROVED.
Comment 12 Devrim GUNDUZ 2007-06-15 17:22:59 EDT
New Package CVS Request
=======================
Package Name: postgresql-pgpoolAdmin
Short Description: Web based pgpool-II administration
Owners: devrim@commandprompt.com
Branches: EL-4 EL-5 FC-6 F-7
InitialCC: devrim@commandprompt.com
Comment 13 Kevin Fenzi 2007-06-15 23:46:43 EDT
cvs done. 
No need to list yourself as owner and in CC. 
Also fedora-cvs should be set to ? for a request, and we set it to + when it's
processed. 

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