[OPEN-ILS-DEV] Script for creating database/updating opensrf.xml database settings

[Dan Scott]

Hi Kevin:

2008/9/23 Kevin Beswick <kevinbeswick00 at gmail.com>:
> Hello fellow listers.
>
> Attached is a script - eg_db_config.pl - which a user can run to
> either update the database settings in their opensrf.xml, or can run
> to retrieve the database settings from opensrf.xml and then run
> 'build-db.sh' with those settings to create the evergreen databases.
>
> This script would reside in 'Open-ILS/src/extras' and can be run with
> any of the following options:
>
> --update-config  --host localhost --port 5432 --dbname evergreen
> --user evergreen --password evergreen
>
> This would update your opensrf.xml with all of the specified values
> (assuming a default location of /openils/conf/opensrf.xml). You can
> choose to only update subsets of those values as well. Also, you can
> manually specify a location for opensrf.xml via:
>
>  --configfile /foo/bar/opensrf.xml
>
> To build the database, just run the script with the following option:
>
> --create-schema
>
> This will run build-db.sh (assuming a default location of
> Open-ILS/src/sql/Pg/build-db.sh). You can manually specify a location
> for build-db.sh by invoking the option:
>
> --builddbfile /foo/bar/build-db.sh
>
>
> Feedback is definitely appreciated as this is my first attempt with Perl.
>
> -Kevin
>

Not bad at all for a first attempt. For one thing, you have use strict
and use warnings - bravo!

A few suggestions for the first revision:

  * If you append parentheses to the subroutine name, it isn't been
necessary to call subroutines using the "&" sigil. Please drop that
from the calls to update_config() and create_schema() (and add
parentheses to the call to the latter).

  * When testing truth values in Perl, there's no need to explicitly
compare against 1... so instead of:

if (!$cschema == 1 && !$uconfig == 1){

most Perl coders will use:

if (!$cschema && !$uconfig){

  * Please give the option to update individual services, rather than
forcing the update of all services; you could maybe use a list
variable and iterate through the specified elements of the list,
prepending the element name to the XPath for the update. And you could
default to updating all services if none are specified, in which case
you can use the existing XPath.

So, an example scenario: Your database is getting hammered by nasty
queries from the reporter module, and it's impacting circulation, so
you replicate the database to shift the reporting load to a dedicated
reporter database. You update the configuration file as follows:

eg_db_config.pl --update-config --service open-ils.cstore --host
newhost --port 5432 --dbname egreporter --user clark --password kent

  * Use GetOpt::Long to handle variables, rather than pulling them
from ARGV. See Open-ILS/src/extras/import/pg_loader.pl and friends for
examples of GetOpt::Long in action

  * Be consistent with word-breaks in options and actions (for
example, use "config-file" instead of "configfile" if everything else
is being hyphenated)

  * Format the help output similar to the output you see with "ls
--help"; probably something like:

eg_db_config.pl [OPTIONS] [--create-schema] [--update-config [CONFIG_OPTIONS]]
Configure Evergreen database settings in the specified file.
(Re)create the Evergreen database schema according to the settings in
the specified file

OPTIONS:
  --config-file    blah blah blah; defaults to blah
  --build-db-file blah blah blah; defaults to blah

ACTIONS:
  --create-schema  (re)creates the Evergreen database schema according
to the settings contained in the open-ils.store section of
--config-file
  --update-config  blah blah

  * Rather than being hardcoded, the default location for opensrf.xml
will come from calling `eg_config --sysconfdir` and concatenating
"opensrf.xml" to it, right?

  * What happens in the case that writing out the XML file fails? It
might make sense to create a backup copy of the XML file before
potentially corrupting it - and inform the user where the backup file
is stored.

  * Variable assignment should be the first thing that occurs in a
subroutine - so move "my @params = @_;" to the first line of the
definition of update_config()

  * Rather than passing a list to update_config(), it would probably
be more flexible to pass a reference to a hash containing only the
key-value pairs that have been passed in at the command line. So,
suppose that a person just updated the username and password for
access to EG - they could type just:

eg_db_config.pl --update-config --user clark --password kent

Then you could iterate using something like:

foreach my $key (keys %$config_parms) {
    my(@node) = $opensrf_config->findnodes("//database/$key/text()");
    foreach (@node) {
        $_->setData($config_parms{$key});
    }
}

... with a little more trickery for per-service configuration, of
course. But not much!

  * In sub create_schema() I don't think there's much value to
building an array of the values you're just going to interpolate into
a command string a few lines later. You might as well use explicit
variable names like $host and $port, which makes the command string
slightly easier to read.

Hmm. Sorry for going on at length, obviously some of this is
relatively minor. Hopefully this helps you on your Perl path, though -
and kbeswick++

-- 
Dan Scott
Laurentian University