Ludovic Courtès writes: > Hello Roman, > > Roman Scherer skribis: > >> * gnu/machine/hetzner.scm: New file. >> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it. >> * guix/ssh.scm (open-ssh-session): Add stricthostkeycheck option. >> * doc/guix.texi (Invoking guix deploy): Add documentation for >> 'hetzner-configuration'. >> >> Change-Id: Idc17dbc33279ecbf3cbfe2c53d7699140f8b9f41 > > Thumbs up for this big piece of work, one that I think is important for > the project! ‘guix deploy’ is a great idea but it desperately needs > more backends like this one. > > I’m not familiar with Hetzner so I’ll comment on more general aspects. > Chris, perhaps you can provide feedback on Hetzner-specific issues? I > think we could put this backend to good use for Guix infra since a few > services are running at Hetzner. > >> +@deftp {Data Type} hetzner-configuration >> +This is the data type describing the server that should be created for a >> +machine with an @code{environment} of @code{hetzner-environment-type}. > > Could you add a sentence providing more context like: > > It allows you to configure deployment to a @acronym{VPS, virtual > private server} hosted by @uref{https://www.hetzner.com, Hetzner}. > >> +@item @code{authorize?} (default: @code{#t}) >> +If true, the coordinator's public signing key > > “coordinator” has nothing to do here I guess. > >> +@item @code{labels} (default: @code{'()}) >> +A user defined alist of key/value pairs attached to the server. Keys and >> +values must be strings. For more information, see >> +@uref{https://docs.hetzner.cloud/#labels, Labels}. > > Maybe add a short example? > >> +@item @code{location} (default: @code{"fsn1"}) >> +The name of a @uref{https://docs.hetzner.com/cloud/general/locations, >> +location} to create the server in. > > Maybe add: “For example, @code{"fsn1"} corresponds to the Hetzner site > in Falkenstein, Germany, while @code{"sin"} corresponds to its site in > Singapore.” > >> +@item @code{server-type} (default: @code{"cx42"}) >> +The name of the >> +@uref{https://docs.hetzner.com/cloud/servers/overview#server-types, >> +server type} this server should be created with. > > Likewise, an example would be elcome. > >> +@item @code{ssh-key} >> +The path to the SSH private key to use to authenticate with the remote >> +host. > > s/path to/file name of/ > >> +The following example shows the definition of 2 machines that are > > s/2/two/ > >> +vCPUs and 32 GB of RAM on the @code{aarch64} architecture, the second > > s/@code{aarch64}/AArch64/ > >> +shared vCPUs and 32 GB of RAM on the @code{x86_64} architecture. > > Drop @code. > >> +@lisp >> +(use-modules (gnu machine) >> + (gnu machine hetzner)) >> + >> +(list (machine >> + (operating-system %hetzner-os-arm) >> + (environment hetzner-environment-type) >> + (configuration (hetzner-configuration >> + (server-type "cax41") >> + (ssh-key "/home/charlie/.ssh/id_rsa")))) >> + (machine >> + (operating-system %hetzner-os-x86) >> + (environment hetzner-environment-type) >> + (configuration (hetzner-configuration >> + (server-type "cpx51") >> + (ssh-key "/home/charlie/.ssh/id_rsa"))))) > > Nice! > >> +API key} should provision 2 machines for you. > > s/2/two/ > >> + #:use-module (ice-9 receive) > > The code base preferable uses SRFI-71 for multiple-value returns. > >> + (raise (formatted-message >> + (G_ "Expected a list of Hetzner API responses"))))) > > Messages should start with a lower-case letter (for all the messages in > this file). > > Please add the file to ‘po/guix/POTFILES.in’ so that it’s actually > subject to translation. > >> +(define (hetzner-api-response-read port) >> + "Read the Hetzner API response from PORT." >> + (let* ((response (read-response port)) >> + (body (read-response-body response))) >> + (hetzner-api-response >> + (body (json-string->scm (bytevector->string body "UTF-8"))) > > Just ‘string->utf8’ (shorter). > > More importantly: instead of ‘json-string->scm’ (which gives an alist, > leading to ‘assoc-ref’ calls all over the code base along with free-form > alists, which is very error-prone), could you use ‘define-json-mapping’? > > In essence it’s like ‘define-record-type’ but it additionally define how > to map a JSON dictionary to a Scheme record. There are several examples > in Guix, such as (guix swh). > > For clarity, it might be useful to move all the hetzner-api-* bits to a > separate module, for example (gnu machine hetzner http). WDYT? > > > The rest of the code looks nice to me (modulo alists :-)) but that’s > about all I can say. It’s quite a significant body of code. What would > you suggest to prevent bitrot and support maintenance? Are there parts > of it that could be usefully tested automatically, possibly by mocking > part of the Hetzner API? Or are there tips on how you tested it that > could be written down in the file itself? > > > Could you move the (guix ssh) bits to a separate patch? > >> +++ b/guix/ssh.scm >> @@ -103,7 +103,8 @@ (define* (open-ssh-session host #:key user port identity >> host-key >> (compression %compression) >> (timeout 3600) >> - (connection-timeout 10)) >> + (connection-timeout 10) >> + (stricthostkeycheck #t)) >> "Open an SSH session for HOST and return it. IDENTITY specifies the file >> name of a private key to use for authenticating with the host. When USER, >> PORT, or IDENTITY are #f, use default values or whatever '~/.ssh/config' > > Please update the docstring. > > Rather ‘strict-host-key-check?’ to match naming conventions, even if > Guile-SSH calls it that way. > >> @@ -137,7 +138,8 @@ (define* (open-ssh-session host #:key user port identity >> >> ;; Speed up RPCs by creating sockets with >> ;; TCP_NODELAY. >> - #:nodelay #t))) >> + #:nodelay #t >> + #:stricthostkeycheck stricthostkeycheck))) > > Not sure what this does actually. Looks like the main part is the > “when stricthostkeycheck” condition that comes below, no? > > Could you send a second version? > > Thank you! > > Ludo’.