GNU bug report logs - #73196
[PATCH] services: postgresql-role: Add support for password files.

Previous Next

Full log

Message #53 received at 73196 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: paul <goodoldpaul <at> autistici.org>
Cc: 73196 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: bug#73196: [PATCH] services: postgresql-role: Add support for
 password files.
Date: Mon, 28 Apr 2025 13:09:11 +0900

Hi,

paul <goodoldpaul <at> autistici.org> writes:

Thanks for addressing most of my comments.  I have a few more ones,
in light of your last replies.

[...]

>>> +  (requirement      postgresql-role-configuration-requirement ;list-of-symbols
>>> +                    (default '()))
>> as mentioned above, this should be 'shepherd-requirement'.
> I know at least two examples that are already in master which use
> 'requirement' (I may be biased as I authored both of them): the
> restic-backup-service-type and the oci-container-service-type. If you
> don't find this to be a blocker I would defer this change to a
> specific commit setting a convention for the whole codebase after an
> analysis of which services use simply 'requirement' and which use
> 'shepherd-requirement'. Is it ok for you?

OK!  I think we'd want to normalize to shepherd-requirement because
'requirement' could well be an option key in a config file (name clash);
'shepherd-requirement', makes this much less likely, while making it
more obvious that this is a shepherd thing exposed through the
configuration object.  For this reason I'd err on using
'shepherd-requirement', because it seems more likely we settle for this
variant and deprecating fields is a bit annoying.

[...]

>>
>>> +        ""))
>>> +
>>>     (define (roles->queries roles)
>>>       (apply mixed-text-file "queries"
>>>              (append-map
>>>               (lambda (role)
>>>                 (match-record role <postgresql-role>
>>>                   (name permissions create-database? encoding collation ctype
>>> -                      template)
>>> +                      template password-file)
>>>                   `("SELECT NOT(EXISTS(SELECT 1 FROM pg_catalog.pg_roles WHERE \
>>>   rolname = '" ,name "')) as not_exists;\n"
>>>   "\\gset\n"
>>>   "\\if :not_exists\n"
>>>   "CREATE ROLE \"" ,name "\""
>>>   " WITH " ,(format-permissions permissions)
>>> +,(if (and (string? password-file)
>>> +          (not (string-null? password-file)))
>> Since you already check for the string-null? case, perhaps the default
>> password value should be "" instead of #f?  And then you could do away
>> with the string? check (though ideally there could be a sanitizer to
>> verify that a string is passed -- perhaps best leaving it for the day
>> this gets ported to define-configuration, which auto-generates sanitizers
>> based on the type specified for the field in its form.
> in its ideal form (using define-configuration) this would be a
> 'maybe-string' type, as it represents an optional path. if you don't
> find this a blocker I would leave this for a later change where we
> could use 'maybe-value-set?' and leave the type check to a
> sanitizer. What do you think?

That's OK!

[...]

>>> -    (log)
>>> +    (log requirement)
>>>       (list (shepherd-service
>>> -           (requirement '(user-processes postgres))
>>> +           (requirement `(user-processes postgres ,@requirement))
>> I see two styles being used in Guix services: 'shepherd-requirement'
>> extend the requirements, or overrides it, with the 'user-processes at
>> least normally found in the default values.  I think at this point in
>> time the later fits better in Guix (think of arguments such as #:modules
>> and #:imported-modules, etc.  the user has full control on them, for the
>> better and worst).
> Thank you for pointing this out, I created a
> %postgresql-role-shepherd-requirement value and exported it to allow
> users which want to override the Guix defaults, while retaining them
> can simply append to this list.

OK!  In my experience it's (somewhat?) conventional to use %default as a
prefix for default values, so perhaps
%default-postgresql-role-shepherd-requirement could be nicer (it's long
but in Scheme the usual style is to not shy away from being extra
descriptive in general).

>>              
>>>              (provision '(postgres-roles))
>>>              (one-shot? #t)
>>>              (start
>>>               #~(lambda args
>>>                   (zero? (spawn-command
>>> -                        #$(postgresql-create-roles config)
>>> +                        (list #$(postgresql-create-roles config))
>> Why is this change now necessary?  I didn't follow.
>
> the return value of postgresql-create-roles changed, before this
> change it returned a list containing a psql command line, after this
> change it returns a gexp which will evaluate to the entrypoint
> responsible for reading password files and then calling psql.

Thanks for explaining.

>>
>>>                           #:user "postgres"
>>>                           #:group "postgres"
>>>                           ;; XXX: As of Shepherd 1.0.2, #:log-file is not
>>> @@ -468,6 +503,7 @@ (define postgresql-role-service-type
>>>                             (match-record config <postgresql-role-configuration>
>>>                               (host roles)
>>>                               (postgresql-role-configuration
>>> +                             (inherit config)
>> and this one?  Was it a preexisting bug?
> Yes, for example without this change we would lose the 'log' field's
> value upon extension.

OK, make sure this is mentioned in the changelog (sorry, I didn't
verify if it's already there).

I'm looking forward to the v7 :-)

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.