curtin-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Review: Needs Fixing

Just one minor nit on the update_nvram defaults being different than each other (and/or docs) if absent.

Diff comments:

> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4afe00c..cde45e2 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -711,38 +690,13 @@ def setup_grub(cfg, target, osfamily=DISTROS.debian):
>      else:
>          instdevs = ["none"]
>  
> -    if uefi_bootable and grubcfg.get('update_nvram', True):
> +    update_nvram = grubcfg.get('update_nvram', True)

Hrm, we still are defaulting to True here for an absent update_nvram, and False in curtin/commands/install_grub.py:install_grub. Don't they both need to align with one default value and also align with docs?

> +    if uefi_bootable and update_nvram:
>          uefi_remove_old_loaders(grubcfg, target)
>  
> -    LOG.debug("installing grub to %s [replace_default=%s]",
> -              instdevs, replace_default)
> +    install_grub(instdevs, target, uefi=uefi_bootable, grubcfg=grubcfg)
>  
> -    with util.ChrootableTarget(target):
> -        args = ['install-grub']
> -        if uefi_bootable:
> -            args.append("--uefi")
> -            LOG.debug("grubcfg: %s", grubcfg)
> -            if grubcfg.get('update_nvram', True):
> -                LOG.debug("GRUB UEFI enabling NVRAM updates")
> -                args.append("--update-nvram")
> -            else:
> -                LOG.debug("NOT enabling UEFI nvram updates")
> -                LOG.debug("Target system may not boot")
> -            if len(instdevs) > 1:
> -                instdevs = [instdevs[0]]
> -                LOG.debug("Selecting primary EFI boot device %s for install",
> -                          instdevs[0])
> -
> -        args.append('--os-family=%s' % osfamily)
> -        args.append(target)
> -
> -        # capture stdout and stderr joined.
> -        join_stdout_err = ['sh', '-c', 'exec "$0" "$@" 2>&1']
> -        out, _err = util.subp(
> -            join_stdout_err + args + instdevs, env=env, capture=True)
> -        LOG.debug("%s\n%s\n", args + instdevs, out)
> -
> -    if uefi_bootable and grubcfg.get('update_nvram', True):
> +    if uefi_bootable and update_nvram:
>          uefi_remove_duplicate_entries(grubcfg, target)
>          uefi_reorder_loaders(grubcfg, target)
>  


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