curtin-dev team mailing list archive

[Dan Bungert <mp+475096@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

The OEM matcher is far too broad and must be fixed before we can proceed.
I want to see a plan that ensure that the things curthooks handles today are handled in the OEM case.
Also, when we inevitably add more things to curthooks, how will OEM adapt to that for these preinstalled images?

Diff comments:

> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4bc3614..46298bd 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -2091,6 +2091,10 @@ def curthooks(args):
>                  description="Configuring Ubuntu-Core for first boot"):
>              ubuntu_core_curthooks(cfg, target)
>          sys.exit(0)
> +    # OEM is special too, handle it.
> +    elif distro.is_ubuntu_oem(target):

Please review all existing curthooks and elaborate on how you're handling those cases in the preinstalled case.  I expect some of them aren't handled, such as the new-to-24.10 case of kdump support, and some earlier ones may also not be handled.

> +        LOG.info('Detected Ubuntu OEM image, skip hooks')
> +        sys.exit(0)
>  
>      # user asked for target, or auto mode
>      if curthooks_mode in ['auto', 'target']:
> diff --git a/curtin/distro.py b/curtin/distro.py
> index e6e13d5..c7639ad 100644
> --- a/curtin/distro.py
> +++ b/curtin/distro.py
> @@ -163,6 +163,11 @@ def is_ubuntu_core_20(target=None):
>      return os.path.exists(target_path(target, 'snaps'))
>  
>  
> +def is_ubuntu_oem(target=None):
> +    """Check if Ubuntu OEM specific file is present at target"""
> +    return os.path.exists(target_path(target, '.disk/info'))

This matches all ISO!s  This is far too broad and would have broken installs if this was merged.  This must be made narrow to OEM.

> +
> +
>  def is_centos(target=None):
>      """Check if CentOS specific file is present at target"""
>      return os.path.exists(target_path(target, 'etc/centos-release'))


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