curtin-dev team mailing list archive

[Dan Watkins <daniel.watkins@xxxxxxxxxxxxx>]

Review: Needs Information

In https://bugs.launchpad.net/maas/+bug/1894217/comments/3, it looks like there are two issues: (a) we're incorrectly detecting duplicates, and (b) we might remove the BootCurrent value.  This branch definitely addresses (b); does it also intend to address (a)?

Diff comments:

> diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
> index ddae280..51df0c1 100644
> --- a/tests/unittests/test_curthooks.py
> +++ b/tests/unittests/test_curthooks.py
> @@ -1035,11 +1035,9 @@ class TestUefiRemoveDuplicateEntries(CiTestCase):
>  
>      @patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
>      def test_uefi_remove_duplicate_entries(self):
> -        cfg = {
> -            'grub': {
> -                'install_devices': ['/dev/vdb'],
> -                'update_nvram': True,
> -            },
> +        grubcfg = {

Would it be clearer that this test does not rely on the contents of this dict if we made it empty instead?

> +            'install_devices': ['/dev/vdb'],
> +            'update_nvram': True,
>          }
>          self.m_efibootmgr.return_value = {
>              'current': '0000',
> @@ -1076,12 +1074,86 @@ class TestUefiRemoveDuplicateEntries(CiTestCase):
>              ], self.m_subp.call_args_list)
>  
>      @patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
> +    def test_uefi_remove_duplicate_entries_disabled(self):
> +        grubcfg = {
> +            'install_devices': ['/dev/vdb'],
> +            'update_nvram': True,
> +            'remove_duplicate_entries': False,
> +        }
> +        self.m_efibootmgr.return_value = {
> +            'current': '0000',
> +            'entries': {
> +                '0000': {
> +                    'name': 'ubuntu',
> +                    'path': (
> +                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
> +                },
> +                '0001': {

This is also a duplicate of 0000, I think; should we annotate it like 0003?  (Do we need both duplicates?)

> +                    'name': 'ubuntu',
> +                    'path': (
> +                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
> +                },
> +                '0002': {  # Is not a duplicate because of unique path
> +                    'name': 'ubuntu',
> +                    'path': (
> +                        'HD(2,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
> +                },
> +                '0003': {  # Is duplicate of 0000
> +                    'name': 'ubuntu',
> +                    'path': (
> +                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
> +                },
> +            }
> +        }
> +
> +        curthooks.uefi_remove_duplicate_entries(grubcfg, self.target)
> +        self.assertEquals([], self.m_subp.call_args_list)
> +
> +    @patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
> +    def test_uefi_remove_duplicate_entries_skip_bootcurrent(self):
> +        grubcfg = {
> +            'install_devices': ['/dev/vdb'],
> +            'update_nvram': True,
> +        }
> +        self.m_efibootmgr.return_value = {

We could consider moving this dict to a variable as it is almost entirely shared between these two tests.

> +            'current': '0003',
> +            'entries': {
> +                '0000': {
> +                    'name': 'ubuntu',
> +                    'path': (
> +                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
> +                },
> +                '0001': {
> +                    'name': 'ubuntu',
> +                    'path': (
> +                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
> +                },
> +                '0002': {  # Is not a duplicate because of unique path
> +                    'name': 'ubuntu',
> +                    'path': (
> +                        'HD(2,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
> +                },
> +                '0003': {  # Is duplicate of 0000
> +                    'name': 'ubuntu',
> +                    'path': (
> +                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
> +                },
> +            }
> +        }
> +
> +        curthooks.uefi_remove_duplicate_entries(grubcfg, self.target)
> +        self.assertEquals([
> +            call(['efibootmgr', '--bootnum=0000', '--delete-bootnum'],
> +                 target=self.target),
> +            call(['efibootmgr', '--bootnum=0001', '--delete-bootnum'],
> +                 target=self.target),
> +            ], self.m_subp.call_args_list)
> +
> +    @patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
>      def test_uefi_remove_duplicate_entries_no_change(self):
> -        cfg = {
> -            'grub': {
> -                'install_devices': ['/dev/vdb'],
> -                'update_nvram': True,
> -            },
> +        grubcfg = {
> +            'install_devices': ['/dev/vdb'],
> +            'update_nvram': True,
>          }
>          self.m_efibootmgr.return_value = {
>              'current': '0000',


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