curtin-dev team mailing list archive

[Michael Hudson-Doyle <mp+437494@xxxxxxxxxxxxxxxxxx>]

Good job on figuring out what's going on here. As usual I think we can clean the situation up a bit more though! Let me know what you think.

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):

So well part of the fun and games here is that the interface for this function is unclear: does it mutate the old config into the new shape or return a config that is in the new shape. Surprise current answer: both! Let's stop that (I think your branch in fact does this but it could be more deliberate about it).

I think the preferable design is to have here a function that extracts the apt config from the global config (so if we deprecated the old formats we could just replace it with "cfg.get('apt')". But I'm happy for you to do something different.

>      """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({})

Uh. You should be making an assertion about the behaviour here? :-)

> +
> +        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()

Not really convinced that this behaviour is a feature but well...

> +
> +    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.