[Bug 200630] Review Request: postgresql_autodoc - PostgreSQL AutoDoc Utility

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: postgresql_autodoc - PostgreSQL AutoDoc Utility


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200630





------- Additional Comments From devrim at commandprompt.com  2006-08-19 16:25 EST -------
Hello,

> Blockers:
> * Package installs to %{_datadir}/pgsql but it neither owns the directory nor
>   depends on anything which requires it.  This may be a bug in the postgresql
>   packaging.  Care to ask Tom Lane if the postgresql package rather than
>   postgresql-server should own %{_datadir}/pgsql?  I don't know the purpose of
>   the directories well enough to judge.

After thinking a bit, I realized that it is my bad: I've changed the dependency.

> * Package does not own %{_datadir}/pgsql/postgresql_autodoc

Fixed.

> Cosmetic:
> * Perl packages typically have virtual provides detailing what perl
>   dependencies they are providing.  The prefered way to Require perl modules
>   is through this virtual provide method.  So instead of
>     BuildRequires: perl-HTML-Template
>     BuildRequires: perl-Pg-DBD
>   you want to have:
>     BuildRequire: perl(DBD::Pg)
>     BuildRequires: perl(HTML::Template)

Fixed.

> * Using %{?dist} in the release makes upgrades across Fedora Core releases
>   work more or less seamlessly.  Consider adding %{?dist} to the end of your
>   Release: tag.

Fixed.

> * The package does not come with a detached license file.  You should ask
>   upstream to include one next time they release a tarball.  (Since the 
> license
>   is included as part of the source code and this is a script so it is in the
>   installed package, this is not a blocker.  But it is convenient for
> end-users to have this.)

Ok, let's keep it for the future.

> * When manually installing files in the spec file you should try to preserve
>   the file timestamps.  This can be done with cp -p in your

Done.

> Questions:
> * If I'm reading the source correctly, this package will only work with
>   postgresql, not other db's that use the perl DBI interface.  But the
>   Requires picked up by rpm do not include perl(DBD::Pg).  Should there
>   be a Requires: perl(DBD:Pg) in the spec?

Yeah, fixed.

> * Running the program just errors for me.  Any clues?
>     $ postgresql_autodoc  -d orchard --password='XXXXX'
>     Can't call method "finish" on an undefined value at
> /usr/bin/postgresql_autodoc line 1203.

Works for me:

# postgresql_autodoc -d test -u postgres --password test  -l
/usr/share/pgsql/postgresql_autodoc/
        (in cleanup) dbd_st_destroy called twice! at /usr/bin/postgresql_autodoc
line 199.
DBI handle 0x9bad4c4 has uncleared implementors data at
/usr/bin/postgresql_autodoc line 199.
    dbih_clearcom (sth 0x9bad4c4, com 0x9e2ec38, imp DBD::Pg::st):
       FLAGS 0x100113: COMSET IMPSET Warn PrintError PrintWarn
       PARENT DBI::db=HASH(0x9baca20)
       KIDS 0 (0 Active)
       IMP_DATA undef
       NUM_OF_FIELDS -1
       NUM_OF_PARAMS 0
Producing test.dia from /usr/share/pgsql/postgresql_autodoc//dia.tmpl
Producing test.dot from /usr/share/pgsql/postgresql_autodoc//dot.tmpl
Producing test.html from /usr/share/pgsql/postgresql_autodoc//html.tmpl
Producing test.neato from /usr/share/pgsql/postgresql_autodoc//neato.tmpl
Producing test.xml from /usr/share/pgsql/postgresql_autodoc//xml.tmpl
Producing test.zigzag.dia from /usr/share/pgsql/postgresql_autodoc//zigzag.dia.tmpl

> * It appears that the only method for providing a password to use when
>   connecting to the database is via the commandline.  This is insecure as it
>   allows another user to see the password with something as simple as the
>   "ps" command.  It would be a very good idea to ask upstream for other methods
>   of sending the password: prompt, config file, etc.

AFAIK it can use environmental variable for the password stuff.

Thanks for the review. The new spec and srpm will be up shortly.

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.