curtin-dev team mailing list archive

[Dan Bungert <mp+443443@xxxxxxxxxxxxxxxxxx>]

One tweak please then LGTM.  See diff comment.

Diff comments:

> diff --git a/curtin/commands/install_grub.py b/curtin/commands/install_grub.py
> index 38bf71a..67e9a52 100644
> --- a/curtin/commands/install_grub.py
> +++ b/curtin/commands/install_grub.py
> @@ -43,41 +44,60 @@ def get_grub_package_name(target_arch, uefi, rhel_ver=None):
>      if 'ppc64' in target_arch:
>          return ('grub-ieee1275', 'powerpc-ieee1275')
>      if uefi:
> -        if target_arch == 'amd64':
> -            grub_name = 'grub-efi-%s' % target_arch
> -            grub_target = "x86_64-efi"
> -        elif target_arch == 'x86_64':
> -            # centos 7+, no centos6 support
> -            # grub2-efi-x64 installs a signed grub bootloader
> -            grub_name = "grub2-efi-x64"
> -            grub_target = "x86_64-efi"
> -        elif target_arch == 'aarch64':
> -            # centos 7+, no centos6 support
> -            # grub2-efi-aa64 installs a signed grub bootloader
> -            grub_name = "grub2-efi-aa64"
> -            grub_target = "arm64-efi"
> -        elif target_arch == 'arm64':
> -            grub_name = 'grub-efi-%s' % target_arch
> -            grub_target = "arm64-efi"
> -        elif target_arch == 'i386':
> -            grub_name = 'grub-efi-ia32'
> -            grub_target = 'i386-efi'
> -        elif target_arch == 'riscv64':
> -            grub_name = 'grub-efi-riscv64'
> -            grub_target = 'riscv64-efi'
> +        if osfamily == distro.DISTROS.redhat:
> +            if target_arch == 'x86_64':
> +                # centos 7+, no centos6 support
> +                # grub2-efi-x64 installs a signed grub bootloader
> +                grub_name = "grub2-efi-x64"
> +                grub_target = "x86_64-efi"
> +            elif target_arch == 'aarch64':
> +                # centos 7+, no centos6 support
> +                # grub2-efi-aa64 installs a signed grub bootloader
> +                grub_name = "grub2-efi-aa64"
> +                grub_target = "arm64-efi"
> +            else:
> +                raise ValueError('Unsupported RHEL version: %s', rhel_ver)
> +        elif osfamily == distro.DISTROS.suse:
> +            if target_arch == 'x86_64':
> +                grub_target = "x86_64-efi"
> +            elif target_arch == 'aarch64':
> +                grub_target = "arm64-efi"
> +            else:
> +                raise ValueError('Unsupported SUSE arch: %s', target_arch)
> +            grub_name = 'grub2-%s' % grub_target
>          else:
> -            raise ValueError('Unsupported UEFI arch: %s' % target_arch)
> +            if target_arch == 'amd64':
> +                grub_name = 'grub-efi-%s' % target_arch
> +                grub_target = "x86_64-efi"
> +            elif target_arch == 'arm64':
> +                grub_name = 'grub-efi-%s' % target_arch
> +                grub_target = "arm64-efi"
> +            elif target_arch == 'i386':
> +                grub_name = 'grub-efi-ia32'
> +                grub_target = 'i386-efi'
> +            elif target_arch == 'riscv64':
> +                grub_name = 'grub-efi-riscv64'
> +                grub_target = 'riscv64-efi'
> +            else:
> +                raise ValueError('Unsupported UEFI arch: %s' % target_arch)

I'd like to see a note here as well for the supplied osfamily.  Like if I call this method intending for osfamily suse but send the value wrong, and include a valid target_arch for suse, I'll end up down on this line and be very confused.

>      else:
>          grub_target = 'i386-pc'
> -        if target_arch in ['i386', 'amd64']:
> -            grub_name = 'grub-pc'
> -        elif target_arch == 'x86_64':
> -            if rhel_ver == '6':
> -                grub_name = 'grub'
> -            elif rhel_ver in ['7', '8', '9']:
> -                grub_name = 'grub2-pc'
> +        if osfamily == distro.DISTROS.redhat:
> +            if target_arch == 'x86_64':
> +                if rhel_ver == '6':
> +                    grub_name = 'grub'
> +                elif rhel_ver in ['7', '8', '9']:
> +                    grub_name = 'grub2-pc'
> +                else:
> +                    raise ValueError('Unsupported RHEL version: %s', rhel_ver)
> +            elif target_arch == 'i386':
> +                grub_name = 'grub-pc'
>              else:
> -                raise ValueError('Unsupported RHEL version: %s', rhel_ver)
> +                raise ValueError('Unsupported RHEL arch: %s', target_arch)
> +        elif osfamily == distro.DISTROS.suse:
> +            grub_name = 'grub2-i386-pc'
> +        elif target_arch in ['i386', 'amd64']:
> +            grub_name = 'grub-pc'
>          else:
>              raise ValueError('Unsupported arch: %s' % target_arch)
>  


-- 
https://code.launchpad.net/~alexsander-souza/curtin/+git/curtin/+merge/443443
Your team curtin developers is requested to review the proposed merge of ~alexsander-souza/curtin:fix_sles_bios_mode into curtin:master.