Re: [pve-devel] [PATCH installer v2 16/17] fix #5536: post-hook: add utility for sending notifications after auto-install

[Christoph Heiss <c.heiss@proxmox.com>]

Thanks for the in-depth review!

On Wed, Jul 24, 2024 at 01:21:11PM GMT, Aaron Lauterer wrote:
> Two few small things inline
>
> On  2024-07-18  15:49, Christoph Heiss wrote:
> > [..]
> > diff --git a/proxmox-post-hook/src/main.rs b/proxmox-post-hook/src/main.rs
> > new file mode 100644
> > index 0000000..d3e5b5c
> > --- /dev/null
> > +++ b/proxmox-post-hook/src/main.rs
> > @@ -0,0 +1,784 @@
> > [..]
> > +/// Holds all the public keys for the different algorithms available.
> > +#[derive(Serialize)]
> > +struct SshPublicHostKeys {
> > +    // ECDSA-based public host key
> > +    ecdsa: String,
> > +    // ED25519-based public host key
> > +    ed25519: String,
> > +    // RSA-based public host key
> > +    rsa: String,
> > +}
> > +
> > +/// A single disk configured as boot disk.
> Is this comment valid this way? AFAIU there was a dedicated struct for boot
> disks in the previous version, but now this struct is for all disks,
> optionally marking it as boot disk.

No, that's really just a leftover from the previous revision, where it
really was just for boot disks. I'll change it appropriately for the
next version.

> > [..]
> > +impl PostHookInfo {
> > +    /// Gathers all needed information about the newly installed system for sending
> > +    /// it to a specified server.
> > +    ///
> > +    /// # Arguments
> > +    ///
> > +    /// * `target_path` - Path to where the chroot environment root is mounted
> > +    /// * `answer` - Answer file as provided by the user
> > +    fn gather(target_path: &str, answer: &Answer) -> Result<Self> {
> > +        println!("Gathering installed system data ..");
> The three dots at the end can most likely be either 3 (or UTF-8 3-dots) or
> just one?

I'd go with three dots, based on that is what we use everywhere else (as
opposed to the UTF-8 ellipsis) and being the same style we use
everywhere else (most notably unconfigured.sh and the
auto-install-assistant) for such "in progress" tasks.

> > +
> > +        let config: InstallConfig =
> > +            serde_json::from_reader(BufReader::new(File::open("/tmp/low-level-config.json")?))?;
> > +
> […]


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel