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

New mountby2 #1307

Merged
merged 3 commits into from
Aug 17, 2022
Merged

New mountby2 #1307

merged 3 commits into from
Aug 17, 2022

Conversation

jreidinger
Copy link
Member

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

Adapt a code to skip newly added ones as there is no request to support them.

Testing

  • Adapted unit tests

@coveralls
Copy link

coveralls commented Aug 17, 2022

Coverage Status

Coverage increased (+0.0008%) to 97.823% when pulling c8c7395 on new_mountby2 into 15a222b on master.

@@ -791,7 +791,7 @@ def encrypt_with_method(method, dm_name, **method_args)
#
# @return [Filesystems::MountByType]
def preferred_mount_by
Filesystems::MountByType.best_for(self, possible_mount_bys)
Filesystems::MountByType.best_for(self, possible_mount_bys & Filesystems::MountByType.all)
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this inconsistent with the way it's done for MountPoint. In MountPoint you redefined #possible_mount_bys to return a filtered list. Here, #possible_mount_bys still returns an unfiltered list and the filter is done at preferred_mount_by.

Copy link
Member Author

@jreidinger jreidinger Aug 17, 2022

Choose a reason for hiding this comment

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

well, reason here is that it is private method and this is its only usage. But I can make it consistent

@@ -90,6 +90,14 @@ def udev_name(value)
end

class << self

alias_method :all_nonfiltered, :all
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent use of nonfiltered vs unfiltered (below, in the MountPoint class).

Copy link
Member Author

Choose a reason for hiding this comment

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

and which one you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, actually what we always do when we override a list or value coming from libstorage-ng is having storage_whatever for the raw value and then whatever for the converted one.

For consistency I guess we should have MountByType.storage_all vs MountByType.all and MountPoint#storage_possible_mount_bys vs MountPoint@possible_mount_bys.

Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

LGTM

@jreidinger jreidinger merged commit 24a7faa into master Aug 17, 2022
@jreidinger jreidinger deleted the new_mountby2 branch August 17, 2022 13:32
@jreidinger jreidinger mentioned this pull request Aug 17, 2022
@yast-bot
Copy link

❌ Public Jenkins job #441 failed

@yast-bot
Copy link

✔️ Internal Jenkins job #228 successfully finished
✔️ Created IBS submit request #277842

@yast-bot
Copy link

✔️ Public Jenkins job #442 successfully finished
✔️ Created OBS submit request #997665

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

Successfully merging this pull request may close these issues.

4 participants