curtin-dev team mailing list archive

[Ryan Harper <rharper@xxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> diff --git a/curtin/storage_config.py b/curtin/storage_config.py
> index 494b142..84825f6 100644
> --- a/curtin/storage_config.py
> +++ b/curtin/storage_config.py
> @@ -981,6 +981,8 @@ class DasdParser(ProbertParser):
>      probe_data_key = 'dasd'
>  
>      def asdict(self, dasd_config):
> +        if dasd_config.get("dasd_type", "ECKD") != "ECKD":

Shouldn't this be 'type'? per the probert dasd dictionary it returns?

> +            return None
>          dasd_name = os.path.basename(dasd_config['name'])
>          device_id = dasd_config['device_id']
>          blocksize = dasd_config['blocksize']
> diff --git a/doc/topics/storage.rst b/doc/topics/storage.rst
> index ebcf8a4..75c5537 100644
> --- a/doc/topics/storage.rst
> +++ b/doc/topics/storage.rst
> @@ -138,11 +145,12 @@ The disk command sets up disks for use by curtin. It can wipe the disks, create
>  partition tables, or just verify that the disks exist with an existing partition
>  table. A disk command may contain all or some of the following keys:
>  
> -**ptable**: *msdos, gpt*
> +**ptable**: *msdos, gpt, vtoc*
>  
>  If the ``ptable`` key is present and a curtin will create an empty
> -partition table of that type on the disk.  Curtin supports msdos and
> -gpt partition tables.
> +partition table of that type on the disk.  On almost all drives,
> +curtin supports msdos and gpt partition tables; ECKD DASD drives on
> +s390x mainframes can only use the "vtoc" partition table.

Good catch on the docs here.

>  
>  **serial**: *<serial number>*
>  
> diff --git a/tests/unittests/test_storage_config.py b/tests/unittests/test_storage_config.py
> index a38f9cd..c895344 100644
> --- a/tests/unittests/test_storage_config.py
> +++ b/tests/unittests/test_storage_config.py
> @@ -792,9 +791,35 @@ class TestDasdParser(CiTestCase):
>              'mode': 'quick',
>              'disk_layout': 'cdl',
>          }
> -        dasd_data = self.dasd.class_data[devname]
> +        dasd_data = self.dasd.class_data["/dev/dasda"]
>          self.assertDictEqual(expected_dict, self.dasd.asdict(dasd_data))
>  
> +    def test_dasd_includes_ECKD(self):
> +        """ DasdParser converts known dasd_data to expected dict. """
> +        expected_dict = {
> +            'type': 'dasd',
> +            'id': 'dasd-dasda',
> +            'device_id': '0.0.1522',
> +            'blocksize': 4096,
> +            'mode': 'quick',
> +            'disk_layout': 'cdl',
> +        }
> +        dasd_data = self.dasd.class_data["/dev/dasda"]
> +        dasd_data['dasd_type'] = "ECKD"

In the probert change, you set the field under a dasd device to 'type'; what converts 'type' to 'dasd_type'?

> +        self.assertDictEqual(expected_dict, self.dasd.asdict(dasd_data))
> +
> +    def test_dasd_skips_FBA(self):
> +        """ DasdParser skips FBA dasds. """
> +        dasd_data = self.dasd.class_data["/dev/dasda"]
> +        dasd_data['dasd_type'] = "FBA"
> +        self.assertIsNone(self.dasd.asdict(dasd_data))
> +
> +    def test_dasd_skips_virt(self):
> +        """ DasdParser skips virt dasds. """
> +        dasd_data = self.dasd.class_data["/dev/dasda"]
> +        dasd_data['dasd_type'] = "virt"
> +        self.assertIsNone(self.dasd.asdict(dasd_data))
> +
>      @skipUnlessJsonSchema()
>      def test_dasd_parser_parses_all_dasd_devs(self):
>          """ DasdParser returns expected dicts for known dasd probe data."""


-- 
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/394224
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:only-eckd-dasds-extract_storage_config into curtin:master.