Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

converting utilitie to object oriented support. #5948

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Naresh-ibm
Copy link
Contributor

Moving the utilities to OOPs supported way to make them more useful and Robust.

@Naresh-ibm Naresh-ibm force-pushed the disk_oops branch 2 times, most recently from 681da63 to d503be1 Compare June 10, 2024 08:03
Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. Because we would avoid the compatibility issues between new and old API.

@Naresh-ibm Naresh-ibm force-pushed the disk_oops branch 2 times, most recently from dcac795 to 549de97 Compare June 12, 2024 04:51
Moving the utilities to OOPs supported way to make them more useful and
Robust.

Signed-off-by: Naresh Bannoth <[email protected]>
@Naresh-ibm
Copy link
Contributor Author

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

  1. That was a mistake while removing other files. I have re-added it
  2. Yes, we are planning to add all disk/block device related libraries under this. for now we have moved disk.py.
  3. I have kept the existing disk.py and will be adding a deprecation massage as you suggest and after few days we can completely move to new package path.

@Naresh-ibm
Copy link
Contributor Author

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'
>>> 

@clebergnu clebergnu self-requested a review July 22, 2024 15:29
@clebergnu
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review Requested
Development

Successfully merging this pull request may close these issues.

3 participants