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

Report warning if space for dump on disk is smaller than requested #30

Conversation

kobliha
Copy link
Member

@kobliha kobliha commented Jun 17, 2015

- FATE #317488: YaST kdump: issue warning, if space for dump is smaller than (RAM + 4 GiB)
- including test cases
"expect" => "
Yast.import \"Kdump\"
Yast::Kdump.free_space_for_dump
",
Copy link
Member

Choose a reason for hiding this comment

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

with fun_ref it can look like

def expectation
  Yast.import "Kdump"
  Kdump.free_space_for_dump
end
...
...
"expect" => fun_ref(method(:expectation), "boolean ()")

this way it is much easier to test your expectation method and easier to read it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jreidinger OK, next time I'll discuss ideas with you :)

Copy link
Member

Choose a reason for hiding this comment

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

To be fair, FunRef documentation is quite terse: FunRef, fun_ref. I'll improve it.

@mvidner
Copy link
Member

mvidner commented Jun 17, 2015

@kobliha Here, an award for the longest branch name so far: 🏆

}
}

warning = Kdump.proposal_warnig
Copy link
Member

Choose a reason for hiding this comment

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

proposal_warniNg, everywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

uuuh, c&p?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, at least it's consistent :)

@kobliha
Copy link
Member Author

kobliha commented Jun 17, 2015

Thank you @mvidner :)

}

warning = Kdump.proposal_warnig
unless warning.empty?
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this is bad practice, similar to description += item unless item.empty?. Adding an empty object works as intended so we don't need the condition.

- _B, _b constant/method suffixes
- better warning text (from Ken)
- different text for little difference between required/available space
- fun_ref in action
@kobliha kobliha force-pushed the report_warning_if_space_for_dump_on_disk_is_smaller_than_requested branch from 2f2911b to 5507969 Compare June 19, 2015 06:25
- After several iterations, this should finally work in running Yast,
  unit tests, and also for Yast Team members as it doesn't use that
  evil eval
@imobachgs
Copy link
Contributor

I don't know if @mvidner has something to say, it looks good to me.


# Smaller differences are not possible to show with quite small precision
# in human-readable size formatting
if (requested_space * 0.99) < free_space && free_space < requested_space
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this. Why the 1%? Doesn't requested_space contain enough buffer already?

Copy link
Member Author

Choose a reason for hiding this comment

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

1%+ difference can be seen in the result of String.FormatSizeWithPrecision(requested_space, 2, true) 2 is a precision

Copy link
Member

Choose a reason for hiding this comment

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

IMHO this is overengineering, accounting for a case where we need 2000MB but have only 1999MB and the text says "We need 2.00GB but have only 2.00GB available". I think nobody will mind, and I will close such bug reports as wontfix.

Please revert this, then it LGTM.

…g reporting "Kdump warns that it needs 8.5 GiB, but 8.5 GB is available" as WONTFIX

- The human-readable form of "size" always fights with rounding
- 8.5 GiB does not need to == 8.5 GiB exactly
- But let's live with that :)
kobliha added a commit that referenced this pull request Jun 19, 2015
…on_disk_is_smaller_than_requested

Report warning if space for dump on disk is smaller than requested
@kobliha kobliha merged commit 956ad9f into yast:master Jun 19, 2015
@kobliha kobliha deleted the report_warning_if_space_for_dump_on_disk_is_smaller_than_requested branch June 19, 2015 14:27
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.

None yet

4 participants