curtin-dev team mailing list archive

[Dan Bungert <mp+439880@xxxxxxxxxxxxxxxxxx>]

Thanks for the MP Alexander.

Before we get too far into review, please understand that the decision to include f2fs in the filesystems dropdown list of Ubuntu-desktop-installer or Subiquity is not mine to make.  Getting support for it in Curtin is just one part.  Look for example at https://github.com/canonical/ubuntu-desktop-installer/issues/1332 - we removed several filesystems from that list.

But if we do want to included f2fs at some point, it starts here, so let's discuss this part.

docs: we should add an entry in the Format command - doc/topics/storage.rst

unittests: please consider the existing tests/unittests/test_block_mkfs.py tests and think about a suitable f2fs version.  tox -e py3-flake8 reports a minor problem in curtin/block/schemas.py

integration tests: curtin has a body of vmtests, but the newer ones we tend to run as part of tests/integration.  I want to see more integration tests instead of new vmtests.  There isn't an example yet in integration on a new filesystem type, so I won't make it required to add one there, but it'd be nice.

Diff comments:

> diff --git a/curtin/block/schemas.py b/curtin/block/schemas.py
> index 3278331..60807d9 100644
> --- a/curtin/block/schemas.py
> +++ b/curtin/block/schemas.py
> @@ -4,7 +4,7 @@ _uuid_pattern = (
>      r'[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}')
>  _path_dev = r'^/dev/[^/]+(/[^/]+)*$'
>  _path_nondev = r'(^/$|^(/[^/]+)+$)'
> -_fstypes = ['btrfs', 'ext2', 'ext3', 'ext4', 'fat', 'fat12', 'fat16', 'fat32',
> +_fstypes = ['btrfs', 'ext2', 'ext3', 'ext4', 'f2fs', 'fat', 'fat12', 'fat16', 'fat32',

Linter will complain the line is too long, let's move an entry or two down a line to keep it happy.

>              'iso9660', 'vfat', 'jfs', 'ntfs', 'reiserfs', 'swap', 'xfs',
>              'zfsroot']
>  _ptable_unsupported = 'unsupported'
> diff --git a/debian/control b/debian/control
> index a35cbf6..0527580 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -25,6 +25,7 @@ Priority: extra
>  Depends: bcache-tools,
>           btrfs-progs | btrfs-tools,
>           dosfstools,
> +         f2fs-tools,

It's not desired to list every filesystem tools package here, curtin installs missing packages needed on demand.    It's better at this point to not list it here, and we can ensure that f2fs-tools is in the right location in the cases that want it.

>           file,
>           gdisk,
>           lvm2,


-- 
https://code.launchpad.net/~nexusprism/curtin/+git/curtin/+merge/439880
Your team curtin developers is requested to review the proposed merge of ~nexusprism/curtin:master into curtin:master.