curtin-dev team mailing list archive

[Dan Bungert <mp+444608@xxxxxxxxxxxxxxxxxx>]

Review: Approve

LGTM but I didn't read the serialization parts too closely.  Let's think about not having that duplicated in the future.

Diff comments:

> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 868fbad..b41c8b0 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -763,19 +768,19 @@ def setup_grub(cfg, target, osfamily, variant):
>                      get_path_to_storage_volume(item_id, storage_cfg_odict))
>  
>          if len(storage_grub_devices) > 0:
> -            if len(grubcfg.get('install_devices', [])):
> +            if grubcfg.install_devices is not None and \
> +               len(grubcfg.install_devices) > 0:

I would personally have written this as `if grubcfg.install_devices:` but I must admit this version is more explicit.  Up to you.

>                  LOG.warn("Storage Config grub device config takes precedence "
>                           "over grub 'install_devices' value, ignoring: %s",
>                           grubcfg['install_devices'])
> -            grubcfg['install_devices'] = storage_grub_devices
> -
> -    LOG.debug("install_devices: %s", grubcfg.get('install_devices'))
> -    if 'install_devices' in grubcfg:
> -        instdevs = grubcfg.get('install_devices')
> -        if isinstance(instdevs, str):
> -            instdevs = [instdevs]
> -        if instdevs is None:
> -            LOG.debug("grub installation disabled by config")
> +            grubcfg.install_devices = storage_grub_devices
> +
> +    LOG.debug("install_devices: %s", grubcfg.install_devices)
> +    if grubcfg.install_devices is None:
> +        LOG.debug("grub installation disabled by config")
> +        instdevs = grubcfg.install_devices
> +    elif len(grubcfg.install_devices) > 0:
> +        instdevs = grubcfg.install_devices
>      else:
>          # If there were no install_devices found then we try to do the right
>          # thing.  That right thing is basically installing on all block
> diff --git a/curtin/commands/install_grub.py b/curtin/commands/install_grub.py
> index 03b4670..2faa2c6 100644
> --- a/curtin/commands/install_grub.py
> +++ b/curtin/commands/install_grub.py
> @@ -411,8 +418,8 @@ def install_grub(devices, target, uefi=None, grubcfg=None):
>          raise ValueError("Invalid parameter 'target': %s" % target)
>  
>      LOG.debug("installing grub to target=%s devices=%s [replace_defaults=%s]",
> -              target, devices, grubcfg.get('replace_default'))
> -    update_nvram = config.value_as_boolean(grubcfg.get('update_nvram', True))
> +              target, devices, grubcfg.replace_linux_default)

An uncaught bug I think?  Very cool.
In case we were wondering about the value of moving away from the dictionaries.

> +    update_nvram = grubcfg.update_nvram
>      distroinfo = distro.get_distroinfo(target=target)
>      target_arch = distro.get_architecture(target=target)
>      rhel_ver = (distro.rpm_get_dist_id(target)


-- 
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/444608
Your team curtin developers is subscribed to branch curtin:master.