curtin-dev team mailing list archive

[Dan Bungert <mp+479606@xxxxxxxxxxxxxxxxxx>]

we'll need a matching merge to master, before we consider this OK to merge here so that we don't lose anything.

> It would be quite nice to use an enum for the bootloaders.

I won't require it here because curtin has one enum at this point (more or less), but we aren't opposed.  Continued use of magic strings I consider a risk, we have found typos in magic strings that meant that curtin config would never apply in certain cases.  Subiquity has some Enums if you want to see how we've done things there.

Do you want these commits squashed together in the series?  I expect the answer is no.  If I move the status to approved up there the merge logic specific to curtin will squash them, so we conventionally mark merge proposals we don't want squashed with "DO NOT SQUASH" in the commit message field.

Diff comments:

> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 48e45d5..3c749a6 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -866,12 +921,7 @@ def update_initramfs(target=None, all_kernels=False):
>      # update-initramfs's -u (update) method.  If the file does
>      # not exist, then we need to run the -c (create) method.
>      boot = paths.target_path(target, 'boot')
> -    for kernel in sorted(glob.glob(boot + '/vmlinu*-*')):
> -        kfile = os.path.basename(kernel)
> -        # handle vmlinux or vmlinuz
> -        kprefix = kfile.split('-')[0]
> -        version = kfile.replace(kprefix + '-', '')
> -        initrd = kernel.replace(kprefix, 'initrd.img')
> +    for kfile, initrd, version in paths.get_kernel_list(target):

kfile isn't used here.  In your opinion do you think get_kernel_list callers need it?

>          # -u == update, -c == create
>          mode = '-u' if os.path.exists(initrd) else '-c'
>          cmd = ['update-initramfs', mode, '-k', version]
> diff --git a/curtin/commands/install_grub.py b/curtin/commands/install_grub.py
> index edc6d33..5c514ee 100644
> --- a/curtin/commands/install_grub.py
> +++ b/curtin/commands/install_grub.py
> @@ -447,31 +444,4 @@ def install_grub(devices, target, uefi=None, grubcfg=None):
>              in_chroot.subp(cmd, env=env, capture=True)
>  
>  
> -def install_grub_main(args):

So while it's not a part of the Subiquity use case, and doesn't appear to be part of MAAS, this is a part of the external interface to curtin and may have users.  Can you explain to me the part about why we probably shouldn't offer this?

If not essential to this MP we could lift this out of the MP and have a side conversation.  In particular, we probably don't need this removal on the release/23.1 branch.

> -    state = util.load_command_environment()
> -
> -    if args.target is not None:
> -        target = args.target
> -    else:
> -        target = state['target']
> -
> -    if target is None:
> -        sys.stderr.write("Unable to find target.  "
> -                         "Use --target or set TARGET_MOUNT_POINT\n")
> -        sys.exit(2)
> -
> -    cfg = config.load_command_config(args, state)
> -    stack_prefix = state.get('report_stack_prefix', '')
> -    uefi = util.is_uefi_bootable()
> -    grubcfg = cfg.get('grub')
> -    with events.ReportEventStack(
> -            name=stack_prefix, reporting_enabled=True, level="INFO",
> -            description="Installing grub to target devices"):
> -        install_grub(args.devices, target, uefi=uefi, grubcfg=grubcfg)
> -    sys.exit(0)
> -
> -
> -def POPULATE_SUBCMD(parser):
> -    populate_one_subcmd(parser, CMD_ARGUMENTS, install_grub_main)
> -
>  # vi: ts=4 expandtab syntax=python


-- 
https://code.launchpad.net/~sjg1/curtin/+git/curtin/+merge/479606
Your team curtin developers is subscribed to branch curtin:release/23.1.