curtin-dev team mailing list archive

[Olivier Gayot <mp+437494@xxxxxxxxxxxxxxxxxx>]

Thanks. I'm leaning towards making translate_old_apt_features mutate the config. Shallow copies are not enough in this context.

Diff comments:

> diff --git a/curtin/commands/apt_config.py b/curtin/commands/apt_config.py
> index f9cce34..d8956e2 100644
> --- a/curtin/commands/apt_config.py
> +++ b/curtin/commands/apt_config.py
> @@ -685,12 +686,10 @@ def apt_command(args):
>      sys.exit(0)
>  
>  
> -def translate_old_apt_features(cfg):
> +def translate_old_apt_features(old_cfg):

My current implementation does a shallow copy of the original configuration when creating the defaultdict object. This is not enough to ensure the original object is intact.

cfg['apt']['primary'] = [{"arches": ["default"],
                          "uri": old.get('ubuntu_archive')}]
cfg['apt']['security'] = [{"arches": ["default"],
                           "uri": old.get('ubuntu_security')}]

This will modify the original object's "apt" dictionary if it already existed.

Going for a deep copy feels a bit wrong so I'm leaning towards "upgrading" the original configuration. The caller can run copy.deepcopy() beforehand if they want to preserve the original config.

>      """translate the few old apt related features into the new config format"""
> -    predef_apt_cfg = cfg.get("apt")
> -    if predef_apt_cfg is None:
> -        cfg['apt'] = {}
> -        predef_apt_cfg = cfg.get("apt")
> +    cfg = collections.defaultdict(dict, old_cfg)
> +    predef_apt_cfg = cfg.get("apt", {})
>  
>      if cfg.get('apt_proxy') is not None:
>          if predef_apt_cfg.get('proxy') is not None:
> diff --git a/tests/unittests/test_apt_source.py b/tests/unittests/test_apt_source.py
> index 9fb3fc5..8609c61 100644
> --- a/tests/unittests/test_apt_source.py
> +++ b/tests/unittests/test_apt_source.py
> @@ -678,6 +678,38 @@ Pin-Priority: -1
>  
>          mockobj.assert_called_with("preferencesfn", expected_content)
>  
> +    def test_translate_old_apt_features(self):
> +        apt_config.translate_old_apt_features({})

Right :)

> +
> +        self.assertEqual(
> +                apt_config.translate_old_apt_features({
> +                    "debconf_selections": "foo"}),
> +                {"apt": {"debconf_selections": "foo"}}
> +        )
> +
> +        self.assertEqual(
> +                apt_config.translate_old_apt_features({
> +                    "apt_proxy": {"http_proxy": "http://proxy:3128"}}),
> +                {"apt": {
> +                    "proxy": {"http_proxy": "http://proxy:3128"},
> +                }}
> +        )
> +
> +    def test_translate_old_apt_features_conflicts(self):
> +        with self.assertRaisesRegex(ValueError, 'mutually exclusive'):
> +            apt_config.translate_old_apt_features({
> +                "debconf_selections": "foo",
> +                "apt": {
> +                    "debconf_selections": "bar",
> +                }})
> +
> +        with self.assertRaisesRegex(ValueError, 'mutually exclusive'):
> +            apt_config.translate_old_apt_features({
> +                "apt_proxy": {"http_proxy": "http://proxy:3128"},
> +                "apt": {
> +                    "proxy": {"http_proxy": "http://proxy:3128"},
> +                }})
> +
>      def test_mirror(self):
>          """test_mirror - Test defining a mirror"""
>          pmir = "http://us.archive.ubuntu.com/ubuntu/";
> diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
> index c13434c..9e4fa87 100644
> --- a/tests/unittests/test_curthooks.py
> +++ b/tests/unittests/test_curthooks.py
> @@ -2268,4 +2268,37 @@ class TestUefiFindGrubDeviceIds(CiTestCase):
>                               self._sconfig(cfg)))
>  
>  
> +class TestDoAptConfig(CiTestCase):
> +    def setUp(self):
> +        super(TestDoAptConfig, self).setUp()
> +        self.handle_apt_sym = 'curtin.commands.curthooks.apt_config.handle_apt'
> +
> +    def test_no_apt_config(self):
> +        with patch(self.handle_apt_sym) as m_handle_apt:
> +            curthooks.do_apt_config({}, target="/")
> +        m_handle_apt.assert_not_called()
> +
> +    def test_apt_config_none(self):
> +        with patch(self.handle_apt_sym) as m_handle_apt:
> +            curthooks.do_apt_config({"apt": None}, target="/")
> +        m_handle_apt.assert_not_called()
> +
> +    def test_apt_config_dict(self):
> +        with patch(self.handle_apt_sym) as m_handle_apt:
> +            curthooks.do_apt_config({"apt": {}}, target="/")
> +        m_handle_apt.assert_called()

Yeah, I agree... but on the other hand, I think a test like this one is a good indication that any value under the "apt" key (except for None) will trigger apt-config stuff.

Otherwise, we can argue that False, empty lists, 0, empty strings and all the falsy values would be good candidate to disable apt-config stuff but I'd rather keep it simple.

> +
> +    def test_with_apt_config(self):
> +        with patch(self.handle_apt_sym) as m_handle_apt:
> +            curthooks.do_apt_config(
> +                    {"apt": {"proxy": {"http_proxy": "http://proxy:3128"}}},
> +                    target="/")
> +        m_handle_apt.assert_called_once()
> +
> +    def test_with_debconf_selections(self):
> +        # debconf_selections are translated to apt config
> +        with patch(self.handle_apt_sym) as m_handle_apt:
> +            curthooks.do_apt_config({"debconf_selections": "foo"}, target="/")
> +        m_handle_apt.assert_called_once()
> +
>  # vi: ts=4 expandtab syntax=python


-- 
https://code.launchpad.net/~ogayot/curtin/+git/curtin/+merge/437494
Your team curtin developers is requested to review the proposed merge of ~ogayot/curtin:curthooks-fix-apt-config-translation into curtin:master.