curtin-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Review: Needs Fixing

Thanks for updating.  I've added some more inline comments. 

>From your last comments:

>  but I had to skip some tests that directly used the network state. However this is not the case for the disabled config test with a version key. Maybe we could always return a null network state for both cases and allow the tests to behave the same

Which unittests?

I wonder if instead of a None, we could add a method/property that indicates if it's disabled; then in places using NetworkState, they could use logic like:

if NetworkState and not NetworkState.disabled:
   foo





Diff comments:

> diff --git a/curtin/commands/apply_net.py b/curtin/commands/apply_net.py
> index ddc5056..5f1bc23 100644
> --- a/curtin/commands/apply_net.py
> +++ b/curtin/commands/apply_net.py
> @@ -99,6 +99,11 @@ def apply_net(target, network_state=None, network_config=None):
>          else:
>              ns = net.parse_net_config_data(netcfg.get('network', {}))
>  
> +            # We admit a None network state if we have a disabled config, since

emit

> +            # this config may not have a version associated with it
> +            if ns is None and netcfg.get('config') == 'disabled':
> +                return

parse_net_config_data will return None for missing 'version' or missing 'config' in the config.

The primary disable config:

network: {config: disabled} 

will result in a None from parse_net_config_data.

The more complete disable config, however won't (return None)

network: {config: disabled, version: 1}

Here we get a non-none NetworkState.  To handle either of these
scenarios we need to use an or:

if ns is None or netconfig.get('config') == 'disabled':
   return

> +
>      if not passthrough:
>          LOG.info('Rendering network configuration in target')
>          net.render_network_state(target=target, network_state=ns)
> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4afe00c..ea835c1 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -1152,7 +1152,10 @@ def detect_required_packages(cfg, osfamily=DISTROS.debian):
>  
>          # skip missing or invalid config items, configs may
>          # only have network or storage, not always both
> -        if not isinstance(cfg.get(cfg_type), dict):
> +        # we are also skipping if the network config is marked as disabled

# skip missing, invalid or disabled config items.  configs may
# only have network or storage, not always both

> +        cfg_type_value = cfg.get(cfg_type)
> +        if (not isinstance(cfg_type_value, dict) or
> +                cfg_type_value.get('config') == 'disabled'):
>              continue
>  
>          cfg_version = cfg[cfg_type].get('version')
> diff --git a/curtin/commands/net_meta.py b/curtin/commands/net_meta.py
> index fdb909e..fe43e66 100644
> --- a/curtin/commands/net_meta.py
> +++ b/curtin/commands/net_meta.py
> @@ -75,6 +75,9 @@ def net_meta(args):
>  
>      # if network-config hook exists in target,
>      # we do not run the builtin
> +    if args.mode == "disabled":

This hunk needs to move down after line 44, the comment in 39/40 is for lines 44-45
We want to always attempt to run an in-target hook, if that's not present then
we can check for disabled config and exit as well.

> +        sys.exit(0)
> +
>      if util.run_hook_if_exists(args.target, 'network-config'):
>          sys.exit(0)
>  


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