Precedence: list
MIME-Version: 1.0
In-Reply-To: <20141121083330.736d8ec5@ncopa-desktop.alpinelinux.org>
References: <1416518609-29322-1-git-send-email-divided.mind@gmail.com> <20141121083330.736d8ec5@ncopa-desktop.alpinelinux.org>
From: =?UTF-8?Q?Rafa=C5=82_Rzepecki?= <divided.mind@gmail.com>
Date: Fri, 21 Nov 2014 13:40:08 -0500
Message-ID: <CAJu-Zix8jx3dmws-bsr7QEYhcSXP-8i6RPJg1aaUMpUYsXprFQ@mail.gmail.com>
Subject: Re: [alpine-devel] [PATCH] testing/pgagent: new aport
To: Natanael Copa <ncopa@alpinelinux.org>
Cc: alpine-devel@lists.alpinelinux.org
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Fri, Nov 21, 2014 at 2:33 AM, Natanael Copa <ncopa@alpinelinux.org> wrot=
e:
>
> On Thu, 20 Nov 2014 16:23:29 -0500
> Rafa* Rzepecki <divided.mind@gmail.com> wrote:
>
> > http://www.pgadmin.org/docs/dev/pgagent.html
> > a job scheduler for PostgreSQL
> > ---
> >  testing/pgagent/APKBUILD      | 64 +++++++++++++++++++++++++++++++++++=
++++++++
> >  testing/pgagent/pgagent.confd | 22 +++++++++++++++
> >  testing/pgagent/pgagent.initd | 25 +++++++++++++++++
> >  3 files changed, 111 insertions(+)
> >  create mode 100644 testing/pgagent/APKBUILD
> >  create mode 100644 testing/pgagent/pgagent.confd
> >  create mode 100644 testing/pgagent/pgagent.initd
>
> Thanks! I applied it, but I have a few questions below.

Thanks! It's my first Alpine package, so I do appreciate the feedback
and will try to fix it.

> > diff --git a/testing/pgagent/APKBUILD b/testing/pgagent/APKBUILD
> > new file mode 100644
> > index 0000000..ae9eaea
> > --- /dev/null
> > +++ b/testing/pgagent/APKBUILD
> > @@ -0,0 +1,64 @@
> > +# Contributor: Rafal Rzepecki <rafal@conjur.net>
> > +# Maintainer:
> > +pkgname=3Dpgagent
> > +pkgver=3D3.4.0
> > +pkgrel=3D0
> > +pkgdesc=3D"a job scheduler for PostgreSQL"
> > +url=3D"http://www.pgadmin.org/docs/dev/pgagent.html"
> > +arch=3D"all"
> > +license=3D"POSTGRESQL"
> > +depends=3D"wxgtk2.8-base postgresql"
>
> What does it need from wxgtk2.8-base that is not pulled in automatic as
> SO (shared object) dependency? Normally abuild will automatically add
> the dependencies for dynamic libs.
>
> I n this case, abuild will automatically add so:libwx_baseu-2.8.so.0 as
> a dependency which (currently) wxgtk2.8-base provides.
>
> I suspect that wxgtk2.8-base can be removed from depends unless I am
> missing something.
>
> Does the server really need to run on the same server as the agent? If
> not, then we should not have a hard dependency for the postgresql
> server.

Yes, you're right; I didn't know what was the established convention
here, the wiki page on creating packages doesn't mention so-deps at
all.

> > diff --git a/testing/pgagent/pgagent.confd b/testing/pgagent/pgagent.co=
nfd
> > new file mode 100644
> > index 0000000..a85be20
> > --- /dev/null
> > +++ b/testing/pgagent/pgagent.confd
> > @@ -0,0 +1,22 @@
> > +# Pool time interval
> > +PGA_POLL=3D"10"
> > +
> > +# Retry period
> > +PGA_RETRY=3D"30"
> > +
> > +# Log file
> > +PGA_LOG=3D"/var/log/pgagent.log"
>
> Having the log file in /var/log means it needs to have root
> permissions. Does the agent really need to run as root?

It doesn't and shouldn't; in fact it should run as postgres in the
case the scripts are used verbatim. Frankly I just copied them from
Gentoo without much thought :) Those things probably should get
reported 'upstream' once we converge on the best solution.

Anyway, what's the preferred way to setup logging in this case? A
post-install script that creates a /var/log subdirectory with proper
permissions?

Also, I'm not entirely sure what the user should be. If the agent is
running on a local postgres, it's most practical (even if not the most
secure) if it runs as 'postgres' user so it has superuser on the
database; if it's not running on the local server, the scripts aren't
generic enough to make it work anyway (auth needs to be set up, which
might be done in a number of different ways -- password, kerberos,
ident, ssl, etc.) so an admin would have to modify the init script
anyway.

> > diff --git a/testing/pgagent/pgagent.initd b/testing/pgagent/pgagent.in=
itd
> > new file mode 100644
> > index 0000000..0e37fcc
> > --- /dev/null
> > +++ b/testing/pgagent/pgagent.initd
> > @@ -0,0 +1,25 @@
> > +#!/sbin/runscript
> > +# Copyright 1999-2012 Gentoo Foundation
> > +# Distributed under the terms of the GNU General Public License v2
> > +# $Header: /var/cvsroot/gentoo-x86/dev-db/pgagent/files/pgagent.initd,=
v 1.1 2012/04/28 18:34:11 titanofold Exp $
> > +
> > +PGAGENTOPT=3D"-t ${PGA_POLL} -r ${PGA_RETRY} -s ${PGA_LOG} -l ${PGA_LE=
VEL}"
> > +
> > +PGOPT=3D"hostaddr=3D${PG_HOST} dbname=3D${PG_DBNAME} user=3D${PG_USER}=
"
> > +
> > +depend() {
> > +    use net
> > +    need postgresql
>
> Does the agent really require that a postgresql server runs on this
> host even if the PG_HOST is pointing to some other host?
>
> I suspect that you can actually run the agent on different host than the
> postgresql server and in that case, you don't need postgresql. It
> should probably say "use postgresql".

Yes, thanks; as mentioned above it's just lifted from Gentoo verbatim.
I agree it should probably have use postgres, but heed the
aforementioned comments about how one would probably modify the script
anyway in anything different than the most common case.

I'm not sure how to approach it best. Ideas are welcome :)

> > +}
> > +
> > +start() {
> > +    ebegin "Starting pgagent"
> > +    start-stop-daemon --start --exec /usr/bin/pgagent -- ${PGAGENTOPT}=
 ${PGOPT}
> > +    eend $?
> > +}
> > +
> > +stop() {
> > +    ebegin "Stopping pgagent"
> > +    start-stop-daemon --stop --name pgagent
> > +    eend $?
> > +}
>
> -nc




--=20
Rafa=C5=82 Rzepecki


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---