[bug#30809] [PATCH 2/2] services: Add Gitolite.

[Clément Lassieur]

Hi Christopher, thank you for the update!

Christopher Baines <address@hidden> writes:

[...]

> +(define gitolite-setup
> +  (match-lambda
> +    (($ <gitolite-configuration> package user group home
> +                                 rc-file admin-pubkey)
> +     #~(let ((user-info (getpwnam #$user)))
> +         (use-modules (guix build utils))
> +
> +         (simple-format #t "guix: gitolite: installing ~A\n" #$rc-file)
> +         (copy-file #$rc-file #$(string-append home "/.gitolite.rc"))
> +
> +         (let ((admin-pubkey #$admin-pubkey)

What's the point of that 'let'?  Afterwards you reuse '$admin-pubkey'
:-).

> +               (pubkey-file #$(string-append home "/id_rsa.pub")))
> +           (when admin-pubkey

If we are 'gitolite-setup', that means 'admin-pubkey' is true, I think,
so that 'when' is useless.

> +             ;; The key must be writable, so copy it from the store
> +             (copy-file #$admin-pubkey pubkey-file)
> +
> +             (chmod pubkey-file #o500)
> +             (chown pubkey-file
> +                    (passwd:uid user-info)
> +                    (passwd:gid user-info))
> +
> +             ;; Set the git configuration, to avoid gitolite trying to use
> +             ;; the hostname command, as the network might not be up yet
> +             (with-output-to-file #$(string-append home "/.gitconfig")
> +               (lambda ()
> +                 (display "[user]
> +        name = GNU Guix
> +        email = address@hidden
> +"))))
> +           ;; Run Gitolite setup, as this updates the hooks and include the
> +           ;; admin pubkey if specified. The admin pubkey is required for
> +           ;; initial setup, and will replace the previous key if run after
> +           ;; initial setup
> +           (let ((pid (primitive-fork)))
> +             (if (eq? pid 0)

I have a slight preference for the previous 'match' expression you used
before, because it's used elsewhere this way and it requires less code.

> +                 (begin

I think that 'begin' is useless.

> +                   ;; Exit with a non-zero status code if an exception is 
> thrown.
> +                   (dynamic-wind
> +                     (const #t)
> +                     (lambda ()
> +                       (setenv "HOME" (passwd:dir user-info))
> +                       (setenv "USER" #$user)
> +                       (setgid (passwd:gid user-info))
> +                       (setuid (passwd:uid user-info))
> +                       (primitive-exit
> +                        (apply system*
> +                               #$(file-append package "/bin/gitolite")
> +                               "setup"
> +                               (if admin-pubkey
> +                                   `("-pk" ,pubkey-file)
> +                                   '()))))
> +                     (lambda ()
> +                       (primitive-exit 1))))
> +                 (waitpid pid)))
> +
> +           (when (file-exists? pubkey-file)
> +             (delete-file pubkey-file)))))))
> +
> +(define (gitolite-activation config)
> +  (if (gitolite-configuration-admin-pubkey config)
> +      (gitolite-setup config)
> +      #~(display
> +         "guix: Skipping gitolite setup as the admin-pubkey has not been 
> provided\n")))

I'm not fan of the idea that a user might:
  1. setup an initial configuration with 'admin-pubkey',
  2. change that configuration once the initial activation has been
     done.

What is the drawback to forcing the user to setup an 'admin-pubkey'?
Maybe you think that doing the activation is annoying and it should only
be done when necessary?  If that's the case, maybe what we need is an
ad-hoc command instead of the activation, a bit like the
'certbot-command' of the Certbot service.

[...]

> +          (test-eq "cloning the admin repository"
> +            #t

test-assert

> +            (invoke #$(file-append git "/bin/git")
> +                    "clone" "-v"
> +                    "ssh://address@hidden:2222/gitolite-admin"
> +                    "/tmp/clone"))
> +
> +          (with-directory-excursion "/tmp/clone"
> +            (invoke #$(file-append git "/bin/git")
> +                    "-c" "user.name=Guix" "-c" "user.email=guix"
> +                    "commit"
> +                    "-m" "Test commit"
> +                    "--allow-empty")
> +
> +            (test-eq "pushing, and the associated hooks"
> +              #t

test-assert

> +              (invoke #$(file-append git "/bin/git") "push")))

Could you confirm that if a hook fails, that test will fail?

> +          (test-end)
> +          (exit (= (test-runner-fail-count (test-runner-current)) 0)))))
> +
> +  (gexp->derivation "gitolite" test))
> +
> +(define %test-gitolite
> +  (system-test
> +   (name "gitolite")
> +   (description "Clone the Gitolite admin repository.")
> +   (value (run-gitolite-test))))

Thanks!
Clément