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

Pass -F to mke2fs for whole disks in RHEL #154

Merged

Conversation

pcahyna
Copy link
Member

@pcahyna pcahyna commented Aug 20, 2020

rebased and changed version of #128

Fixes #122

@pcahyna pcahyna requested a review from dwlehman August 20, 2020 22:55
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 20, 2020

This pull request introduces 1 alert when merging 951e1c8 into ab163df - view on LGTM.com

new alerts:

  • 1 for Unused import

@pcahyna pcahyna force-pushed the el7-ext4-whole-disk-rebased branch from 951e1c8 to b4029ed Compare August 20, 2020 23:02
@pcahyna
Copy link
Member Author

pcahyna commented Aug 20, 2020

it is WIP because I need to document the new option, but the code should be ready.

@@ -6,3 +6,7 @@ blivet_package_list:
- libblockdev-lvm
- libblockdev-mdraid
- libblockdev-swap
__storage_blivet_mkfs_option_map:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only BlivetDiskVolume uses this map. Shouldn't the map also have something to indicate which volume type the arguments apply to?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, I'll change the name, of the module parameter as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in last commit.

@pcahyna pcahyna changed the title WIP: rebased version of #128 with suggested change Pass -F to mke2fs for whole disks in RHEL Aug 21, 2020
pcahyna and others added 4 commits August 21, 2020 14:34
With merging of linux-system-roles#97 the error message when resizing a volume to a too large
size got changed. Account for that in the test.
More descriptive name both for Ansible and Python variables, and
add documentation
@pcahyna pcahyna force-pushed the el7-ext4-whole-disk-rebased branch from ae61f10 to c22e0c8 Compare August 21, 2020 12:40
@pcahyna
Copy link
Member Author

pcahyna commented Aug 21, 2020

Rebased on top of #158 to make tests pass.

@pcahyna pcahyna requested review from dwlehman and japokorn August 21, 2020 14:17
Copy link
Collaborator

@dwlehman dwlehman left a comment

Choose a reason for hiding this comment

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

I don't love it, largely because it suggests we would need to add a new map and parameter for each volume/pool type that needs such tweaks. It does have the advantage of being mostly contained in user-friendly yaml. I think if we need to improve it somehow later we can do that, but this is fine as a solution to the immediate problem of rhel7 mke2fs on whole disks.

@dwlehman
Copy link
Collaborator

It's also worth noting that there are no tests.

@pcahyna
Copy link
Member Author

pcahyna commented Aug 21, 2020

I don't find it pretty either. For me the main advantage over your previous method is that it is using exactly the same mechanism for determining the distribution/version specific override as for any other distribution/version specific value. If this logic is hidden inside the module, it looks better, but there is some chance that in some corner case the logic inside the module will detect another distribution/version than the logic of the role.

@dwlehman
Copy link
Collaborator

Yeah, consistency is good.

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.

storage: format entire disk with ext4 fs on RHEL7 will be failed
2 participants