Add proposal initial strategies #384
Conversation
src/lib/y2storage/guided_proposal.rb
Outdated
| proposal | ||
| # The strategy depends on the settings format. | ||
| # | ||
| # @see ProposalSeetings#format |
There was a problem hiding this comment.
typo: Seetings instead of Settings.
| # Try proposal with initial settings | ||
| current_settings = settings || ProposalSettings.new_for_current_product | ||
| log.info("Trying proposal with initial settings: #{current_settings}") | ||
| proposal = try_proposal(current_settings.dup, devicegraph, disk_analyzer) |
There was a problem hiding this comment.
Watch out when cloning objects. Take into account that it is not a deep-clone (maybe in this case it is just fine):
orig = Y2Storage::ProposalSettings.new_for_current_product
copy = orig.dup
puts copy.subvolumes.size #=> 22
orig.subvolumes.shift
puts copy.subvolumes.size #=> 21
In YaST we have a deep_copy, so it should work:
copy = Yast.deep_copy(orig)Using the Marshall Ruby module should also work:
copy = Marshal.load(Marshal.dump(orig))If in this case it is not a problem, please document such behavior.
| log.info("Trying proposal with initial settings: #{current_settings}") | ||
| proposal = try_proposal(current_settings.dup, devicegraph, disk_analyzer) | ||
|
|
||
| # Try proposal without home |
There was a problem hiding this comment.
np: Maybe you could move each try to a different method (try_proposal_without_home, etc.) to avoid this method growing too much.
| proposal = try_without_proposed(proposal, current_settings, volume, | ||
| devicegraph, disk_analyzer) | ||
|
|
||
| next |
There was a problem hiding this comment.
rubocop said yes, but not now :S.
|
|
||
| return proposal if !proposal.failed? || volume.nil? | ||
|
|
||
| proposal = try_without_adjust_by_ram(proposal, current_settings, volume, |
There was a problem hiding this comment.
I see each try in a different method here 👍
| using Y2Storage::Refinements::SizeCasts | ||
|
|
||
| def volume_settings(settings, mount_point) | ||
| settings.volumes.detect { |v| v.mount_point == mount_point } |
There was a problem hiding this comment.
np: use #find instead of #detect (see https://github.com/bbatsov/ruby-style-guide#map-find-select-reduce-size).
| let(:root_snapshots_size) { 5.GiB } | ||
|
|
||
| it "makes a valid proposal without changing settings" do | ||
| expect(proposal.failed?).to be(false) |
There was a problem hiding this comment.
np: You can write expect(proposal).to be_failed. Just a suggestion: use which reads better to you. The same applies to the rest, although expect(root_settings).to be_snapshot is weird.
There was a problem hiding this comment.
I do not know why, but I prefer like it is now :)
| end | ||
|
|
||
| context "and it is possible without separate home" do | ||
| it "makes a valid proposal only deactivating separate home" do |
There was a problem hiding this comment.
np: I know sometimes it is hard to organize context/it blocks, especially in a case like this. I am a big fan of nesting contexts :) but maybe here you could rephraseit. By now, it is not an issue. But what if we add two more possibilities?
context "when settings are not given" do # I prefer the word 'given' here
it "creates initial proposal..."
end
context "when settings are given" do
it "makes..."
context "when it is not possible to create a proposal using current settings" do
it "makes a valid proposal without separate home"
end
context "when it is not possible to create a proposal using a separate home" do
it "makes a valid proposal deactivating snapshots"
end
context "when it is not possible even without snapshots" do
it "does not make a valid proposal"
end
endThere was a problem hiding this comment.
Yes, much better.
| let(:swap_max_size) { 10.GiB } | ||
| let(:swap_disable_order) { 1 } | ||
|
|
||
| it "tries without adjust_by_ram" do |
There was a problem hiding this comment.
I do not think this test adds some value respect of the next one. I would merge both of them and I will remove the context. Same for the tests below.
| end | ||
| end | ||
|
|
||
| context "and disable_order implies to dactivate big volumes first" do |
PBI: https://trello.com/c/I9K56rmD/566-5-allow-to-propose-a-data-partition-changes-in-the-proposal-logic