yast / yast-storage-ng

Rewrite of https://github.com/yast/yast-storage
http://yast.github.io/
GNU General Public License v2.0
14 stars 20 forks source link

New mountby #1306

Closed jreidinger closed 2 years ago

jreidinger commented 2 years ago

Problem

Libstorage-ng introduces new types for mount_by which causes failure in test suites and also other code is not adapted for it.

Solution

Introduce list of supported mount by and skip newly added ones as there is no request to support them.

Testing

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.0003%) to 97.822% when pulling b5b69c8dd7648c91f93556f601aff193c8aa0f1b on new_mountby into 15a222b92c2ea6c6eff44b150d7094640e5dabb2 on master.

ancorgs commented 2 years ago

I would prefer adapting yast2-storage-ng in a way that actually makes the new undesired types invisible to the users of Y2Storage.

This actually implies changing all the calls everywhere from MountByType.all into calls to MountByType.all_supported, including the calls in the test suite. That's a modification in the API.

In my opinion, the correct fix should imply no changes in the testsuite.

In other words, I would modify MountByType.all to keep returning the same result that it returned before the change in libstorage-ng.

mvidner commented 2 years ago

In other words, I would modify MountByType.all to keep returning the same result that it returned before the change in libstorage-ng.

I tried this but that alone did not fix the tests:

diff --git a/src/lib/y2storage/filesystems/mount_by_type.rb b/src/lib/y2storage/filesystems/mount_by_type.rb
index d3f64ebe..a50278e6 100644
--- a/src/lib/y2storage/filesystems/mount_by_type.rb
+++ b/src/lib/y2storage/filesystems/mount_by_type.rb
@@ -36,6 +36,10 @@ module Y2Storage

       wrap_enum "MountByType"

+      def self.all
+        super - [PARTLABEL, PARTUUID]
+      end
+
       # Hash with the properties for each mount by type.
       #
       # Keys are the symbols representing the types and values are hashes that
jreidinger commented 2 years ago

@mvidner I think that you cannot use super for wrapped class.

@ancorgs well, it can be done that way. What I do not like about it is that it is not consistent as for other classes all is all what is supported by libstorage-ng and here it will be reduced. But if you are OK with it, I can redo it.

ancorgs commented 2 years ago

Actually what I found inconsistent with other classes is using WhateverEnum.all (eg. DasdType.all, EncryptionType.all, LvType.all, etc.) in all of them and then having to useMountByType.all_supported`.

jreidinger commented 2 years ago

resolved in https://github.com/yast/yast-storage-ng/pull/1307