curtin-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Thanks, a couple more comments inline.

Diff comments:

> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4afe00c..e0498f6 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -1155,7 +1155,7 @@ def detect_required_packages(cfg, osfamily=DISTROS.debian):
>          if not isinstance(cfg.get(cfg_type), dict):
>              continue
>  
> -        cfg_version = cfg[cfg_type].get('version')
> +        cfg_version = cfg[cfg_type].get('version', 1)

Let's just add 'config: disabled' as a skip, check before the version

if not isinstance(cfg.get(cfg_type), dict) or cfg.get(cfg_type) == 'disabled':
    continue

>          if not isinstance(cfg_version, int) or cfg_version not in cfg_map:
>              msg = ('Supplied configuration version "%s", for config type'
>                     '"%s" is not present in the known mapping.' % (cfg_version,
> diff --git a/curtin/commands/net_meta.py b/curtin/commands/net_meta.py
> index fdb909e..0454d3f 100644
> --- a/curtin/commands/net_meta.py
> +++ b/curtin/commands/net_meta.py
> @@ -134,10 +137,11 @@ def net_meta(args):
>  
>      if not target:
>          raise Exception(
> -            "No target given for mode = '%s'.  No where to write content: %s" %
> +            "No target given for mode = %s. Nowhere to write content: %s" %

we can leave the ' ' around the mode string

>              (args.mode, content))
>  
> -    LOG.debug("writing to file %s with network config: %s", target, content)
> +    LOG.debug(
> +        "writing to file %s with network config: %s", target, content)

Why change this?

>      if target == "-":
>          sys.stdout.write(content)
>      else:
> diff --git a/curtin/net/__init__.py b/curtin/net/__init__.py
> index ef2ba26..0fdd2f2 100644
> --- a/curtin/net/__init__.py
> +++ b/curtin/net/__init__.py
> @@ -251,6 +251,7 @@ def parse_net_config_data(net_config):
>      :param net_config: curtin network config dict
>      """
>      state = None
> +    net_config.setdefault('version', 1)

Let's not set this;  In net_apply.net_apply()  if we call net.parse_net_config_data() and we get a None for network_state, then we can return.  NetworkState() on a network: config: disabled should be None.

>      if 'version' in net_config and 'config' in net_config:
>          ns = network_state.NetworkState(version=net_config.get('version'),
>                                          config=net_config.get('config'))
> diff --git a/curtin/net/deps.py b/curtin/net/deps.py
> index fd9e3c0..905dbfd 100644
> --- a/curtin/net/deps.py
> +++ b/curtin/net/deps.py
> @@ -21,8 +21,10 @@ def network_config_required_packages(network_config, mapping=None):
>      if 'network' in network_config:
>          network_config = network_config.get('network')
>  
> +    if network_config.get('config') == 'disabled':
> +        dev_configs = set()
>      # v1 has 'config' key and uses type: devtype elements
> -    if 'config' in network_config:
> +    elif 'config' in network_config:

It does seem strange to do a .get('config') and then an later check if 'config' is in the dictionary;  That's why I used the ternary.  If we want to use if/else clauses then:

if 'config' in network_config:
    if network_config['config'] == 'disabled'
        dev_configs = set()
    else:
        dev_configs = set(....)
else:
  # handle v2 here

I think the nested if/else clause is more compact with the ternary.

>          dev_configs = set(device['type']
>                            for device in network_config['config'])
>      else:
> diff --git a/curtin/net/network_state.py b/curtin/net/network_state.py
> index ab0f277..34abd00 100644
> --- a/curtin/net/network_state.py
> +++ b/curtin/net/network_state.py
> @@ -21,7 +21,12 @@ def from_state_file(state_file):
>  class NetworkState:
>      def __init__(self, version=NETWORK_STATE_VERSION, config=None):
>          self.version = version
> -        self.config = config
> +
> +        if config is None or config == 'disabled':
> +            self.config = []
> +        else:
> +            self.config = config
> +

A ternary here works as well:

self.config = [] if config in [None, 'disabled'] else config

>          self.network_state = {
>              'interfaces': {},
>              'routes': [],
> @@ -73,6 +78,7 @@ class NetworkState:
>      def parse_config(self):
>          # rebuild network state
>          for command in self.config:
> +            # If network is disabled, there is no command to be executed

I don't think we need this here, if anything move this up to the __init__ method where we assign self.config

>              handler = self.command_handlers.get(command['type'])
>              handler(command)
>  
> @@ -379,7 +385,7 @@ if __name__ == '__main__':
>      from curtin import net
>  
>      def load_config(nc):
> -        version = nc.get('version')
> +        version = nc.get('version', 1)

Let's not set version default to 1

>          config = nc.get('config')
>          return (version, config)
>  
> diff --git a/tests/unittests/test_commands_net_meta.py b/tests/unittests/test_commands_net_meta.py
> new file mode 100644
> index 0000000..4f5d92b
> --- /dev/null
> +++ b/tests/unittests/test_commands_net_meta.py
> @@ -0,0 +1,117 @@
> +# This file is part of curtin. See LICENSE file for copyright and license info.
> +
> +import os
> +
> +from mock import MagicMock, call
> +
> +from .helpers import CiTestCase, simple_mocked_open
> +
> +from curtin.commands.net_meta import net_meta
> +
> +
> +class NetMetaTarget:
> +    def __init__(self, target, mode=None, devices=None):
> +        self.target = target
> +        self.mode = mode
> +        self.devices = devices
> +
> +
> +class TestNetMeta(CiTestCase):
> +
> +    def setUp(self):
> +        super(TestNetMeta, self).setUp()
> +
> +        patches = [
> +            ('curtin.util.run_hook_if_exists', 'm_run_hook'),
> +            ('curtin.util.load_command_environment', 'm_command_env'),
> +            ('curtin.config.load_command_config', 'm_command_config'),
> +            ('curtin.config.dump_config', 'm_dump_config'),
> +            ('os.environ', 'm_os_environ')
> +        ]
> +
> +        for (tgt, attr) in patches:
> +            self.add_patch(tgt, attr)

We typically just list them out:

self.add_patch('curtin.util.run_hook_if_exists', 'm_run_hook')
self.add_patch('curtin.util.load_command_environment', 'm_command_env')

etc, it's more compact (vertically)

> +
> +        self.args = NetMetaTarget(
> +            target='net-meta-target'
> +        )
> +
> +        self.base_network_config = {
> +            'network': {
> +                'version': 1,
> +                'config': {
> +                    'type': 'physical',
> +                    'name': 'interface0',
> +                    'mac_address': '52:54:00:12:34:00',
> +                    'subnets': {
> +                        'type': 'dhcp4'
> +                    }
> +                }
> +            }
> +        }
> +
> +        self.disabled_network_config = {
> +            'network': {
> +                'version': 1,
> +                'config': 'disabled'
> +            }
> +        }
> +
> +    def test_net_meta_with_disabled_network(self):
> +        output_network_path = self.tmp_path('/tmp/my-network-config')

this is the same for all tests, you can move this to setUp()

self.output_network_path = self.tmp_path('my-network-config')

Note that tmp_path already makes it in a temporary path, so you can
just provide a filename.

> +        expected_exit_code = 0
> +        self.m_run_hook.return_value = False
> +        self.m_command_env.return_value = {}
> +        self.m_command_config.return_value = self.base_network_config
> +        self.m_os_environ.get.return_value = output_network_path
> +        self.args.mode = 'disabled'

I typically set the "happy" path return_values in setUp() and then in each specific test case, only make a change to the default value for the specific scenario.  This reduces the amount of code duplication between each test-case.

> +
> +        with self.assertRaises(SystemExit) as cm:
> +            with simple_mocked_open(content='') as m_open:
> +                net_meta(self.args)
> +
> +        self.assertEqual(expected_exit_code, cm.exception.code)
> +        self.assertEqual(0, self.m_run_hook.call_count)
> +        self.assertEqual(0, self.m_command_env.call_count)
> +        self.assertEqual(0, self.m_command_config.call_count)
> +
> +        self.assertEquals(self.args.mode, 'disabled')
> +        self.assertEqual(0, self.m_os_environ.get.call_count)
> +        self.assertEqual(0, self.m_dump_config.call_count)
> +        self.assertFalse(os.path.isfile(output_network_path))
> +        self.assertEqual(0, m_open.call_count)
> +
> +    def test_net_meta_with_config_network(self):
> +        output_network_path = self.tmp_path('/tmp/my-network-config')
> +        network_config = self.base_network_config
> +        dump_content = 'yaml-format-network-config'
> +        expected_m_command_env_calls = 2
> +        expected_m_command_config_calls = 2
> +        expected_exit_code = 0
> +        m_file = MagicMock()
> +
> +        self.m_run_hook.return_value = False
> +        self.m_command_env.return_value = {}
> +        self.m_command_config.return_value = network_config
> +        self.m_os_environ.get.return_value = output_network_path
> +        self.m_dump_config.return_value = dump_content
> +
> +        with self.assertRaises(SystemExit) as cm:
> +            with simple_mocked_open(content='') as m_open:
> +                m_open.return_value = m_file
> +                net_meta(self.args)
> +
> +        self.assertEqual(expected_exit_code, cm.exception.code)
> +        self.m_run_hook.assert_called_with(
> +            self.args.target, 'network-config')
> +        self.assertEquals(self.args.mode, 'custom')
> +        self.assertEqual(
> +            expected_m_command_env_calls, self.m_command_env.call_count)
> +        self.assertEqual(
> +            expected_m_command_config_calls, self.m_command_env.call_count)
> +        self.m_dump_config.assert_called_with(network_config)
> +        self.assertEqual(
> +            call(output_network_path, 'w'), m_open.call_args_list[-1])
> +        self.assertEqual(
> +            call(dump_content),
> +            m_file.__enter__.return_value.write.call_args_list[-1])


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