Open Naresh-ibm opened 5 months ago
Hi @Naresh-ibm thank you for your contribution. This is a huge change and I think I don't know the whole story behind this, therefore I have a couple of questions and comments which I need to discuss.
1. I don't see any substitution for `distro` utility, what are the reasons for removing that? 2. Why are you creating the `block_devices` package? Are you planing to add more utils to that? Because if not, IMO the disk util cans stay where it is. If yes, certainly the `block_devices` should be in separated commit with the explanation of future plans. 3. Even tho I understand the benefits of moving this to OOPs I am not sure about it, because it breaks the API which is used by our users, and it will definitely break many test suites. Therefore, I would propose together with this change keep the old API with the warning about deprecation in future releases and remove it after a release 107. 4. Maybe we should wait a little bit with this change and do it during the [migration to autils](https://avocado-framework.readthedocs.io/en/latest/blueprints/BP005.html). Because we would avoid the compatibility issues between new and old API.
Hi @richtja Thanks for your review points
sample logs
>>> disk.freespace('/dev/nvme0n1')
4194304
>>> obj = disk.DiskUtils('/dev/nvme0n1', '/mnt')
>>> obj.freespace()
4194304
>>>
>>> obj.get_disk_blocksize()
65536
>>> disk.get_disk_blocksize('/dev/nvme0n1')
65536
>>>
>>> obj = disk.DiskUtils('/dev/nvme0n1', '/mnt')
>>> obj.delete_loop_device('/dev/loop0')
True
>>>
>>>
>>> disk.create_loop_device(512)
'/dev/loop0'
>>> obj.delete_loop_device('/dev/loop0')
True
>>>
>>> disk
disk
>>> disk.create_loop_device(512)
'/dev/loop0'
>>>
>>> disk.delete_loop_device('/dev/loop0')
True
>>>
>>> obj.get_absolute_disk_path()
'/dev/nvme0n1'
>>> disk.get_absolute_disk_path('/dev/nvme0n1')
'/dev/nvme0n1'
>>>
>>> disk.get_absolute_disk_path('nvme0n1')
'/dev/nvme0n1'
>>>
>>> obj.get_absolute_disk_path()
'/dev/nvme0n1'
>>>
>>> obj.get_available_filesystems()
['autofs', 'pstore', 'proc', 'btrfs', 'fuseblk', 'bdev', 'fuse', 'tracefs', 'mqueue', 'tmpfs', 'ramfs', 'sockfs', 'configfs', 'cpuset', 'cgroup2', 'cgroup', 'hugetlbfs', 'securityfs', 'fusectl', 'pipefs', 'debugfs', 'sysfs', 'devpts', 'devtmpfs', 'bpf']
>>>
>>> obj.get_available_filesystems()
['autofs', 'pstore', 'proc', 'btrfs', 'fuseblk', 'bdev', 'fuse', 'tracefs', 'mqueue', 'tmpfs', 'ramfs', 'sockfs', 'configfs', 'cpuset', 'cgroup2', 'cgroup', 'hugetlbfs', 'securityfs', 'fusectl', 'pipefs', 'debugfs', 'sysfs', 'devpts', 'devtmpfs', 'bpf']
>>>
>>> obj.get_filesystem_type()
'btrfs'
>>>
>>> disk.get_filesystem_type()
'btrfs'
>>>
Hi @Naresh-ibm, first of all, thanks for your work here.
IIUC, the first versions removed the original version of the module, while this keeps both. Is that the intention? If so, I have to ask: how can/should/will two copies of basically the same code, although with a different programming paradigm, be maintained?
That is a tough question for me to answer, and I'm assuming it would be tough for you to answer too. So, without a clear answer and path forward, it's hard to agree with the inclusion of a "duplicate" of sorts.
But, there's one solution.
We're working on a better version of the Avocado utilities, which is the https://github.com/avocado-framework/autils project/repo. That would be the perfect place to introduce libraries such as this: better versions of existing ones. In that process, we can go over what will be the chosen paradigm of the utilities, so that all have a common look and feel. Although that's a very hard thing to do (just think of the Python standard library itself), I believe we can make significant progress since we're starting fresh and have a lot of accumulated knowledge.
Please let me know if you're willing to further work on this contribution, but targeting autils instead. BTW, it's very important that you read the autils BluePrint.
Moving the utilities to OOPs supported way to make them more useful and Robust.