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

lvm.lv present accept mb, gb, tb, pb as size - Fix issue 58985 #59610

Conversation

piterpunk
Copy link

What does this PR do?

  • Updated the _convert_to_mb to handle units ending in "B", like "MB" or "GB"
  • Made a more resilient error handling for invalid size formats
  • Clarified in documentation what is expected by the "size" parameter
  • Updated the tests with the new use cases

What issues does this PR fix or reference?

Fixes: #58985

Previous Behavior

lvm.lv_present fails ungracefully when an unknown size unit is used. Regression when using previously accepted MB, GB, TB, etc. Documentation is not clear about what are the accepted units.

New Behavior

Now lvm.lv_present documentation presents the allowed units and makes clear that the default unit is in megabytes. Reverted the regression with MB, GB, TB, etc and internally converts then to M, G and T. Made a better error handling in unit convertion raising a more clear message when needed.

Merge requirements satisfied?

Commits signed with GPG?

No

piterpunk added 2 commits February 24, 2021 22:00
- Updated the _convert_to_mb to handle units ending in "B", like
  "MB" or "GB"
- Made a more resilient error handling for invalid size formats
- Clarified in documentation what is expected by the "size" parameter
- Updated the tests with the new use cases
@piterpunk piterpunk requested a review from a team as a code owner February 25, 2021 01:16
@piterpunk piterpunk requested review from Ch3LL and removed request for a team February 25, 2021 01:16
@piterpunk
Copy link
Author

piterpunk commented Mar 2, 2021

@sagetherage Hi, this fixes ##58985 which is marked for Aluminum, can it be merged for this release cycle instead to wait for the point release?

@sagetherage
Copy link
Contributor

@sagetherage Hi, this fixes ##58985 which is marked for Aluminum, can it be merged for this release cycle instead to wait for the point release?

working on it

@sagetherage sagetherage added the Aluminium Release Post Mg and Pre Si label Mar 2, 2021
@Ch3LL Ch3LL merged commit d9cc4fb into saltstack:master Mar 3, 2021
@piterpunk piterpunk deleted the lvm.lv_present-accept-MB_GB_TB_PB-as-size_Fix-Issue-58985 branch March 8, 2021 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] lvm.lv_present doesn't like MB any more, documentation unclear
5 participants