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

Fix use #411

Merged
merged 8 commits into from
Nov 3, 2017
Merged

Fix use #411

merged 8 commits into from
Nov 3, 2017

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Nov 3, 2017

This fixes an important bug: bsc#1066398.

Basically, AutoinstSpaceMaker kicks in when partitions have been planned and does not remove any partition that could be reused.

The tricky part is mapping the sid with the device name in order to reassign reuse properties after making space (as, for instance, removing a logical partition might cause other partitions to be renamed).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 95.647% when pulling 4725d5a on fix-use into 735e9eb on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 95.688% when pulling 4faced0 on fix-use into 735e9eb on master.

Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

There is conflict (with changelog and version number, I guess).

all << devicegraph.partitions.find { |p| device.reuse == p.name }
when Y2Storage::Planned::LvmVg
vg = devicegraph.lvm_vgs.find { |v| File.join("/dev", device.reuse) == v.name }
all.concat(vg.lvm_pvs.map(&:blk_device))
Copy link
Contributor

Choose a reason for hiding this comment

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

Method LvmPv#blk_device could return the encryption device instead of the blk device. There is no problem if the device hosting the PV is a partition due to you are searching then by partitions in the ancestors list. But there is a possible corner case: a PV over an encrypted disk. Is it necessary to take into account that scenario? Probably not because you are only working with partitions here, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info. Yes, I am working with partitions. If I can reach the partition through the "#ancestors" method, it will be ok. If not, I guess I should handle it.

Y2Storage::Planned::Partition.new("/").tap { |p| p.reuse = "/dev/sda1" }
end

it "does not removes the partition table" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: removes --> remove

* master:
  Bump version
  Improve Devicegraph#remove_md
  Fix text and doc
  Fix doc
  Update version and changelog
  Update specs
  Remove LvmResizeButton
  Add resize and delete buttons
  Add DeviceResizeButton
  Add support to delete button for deleting mds
  Add Actions::DeleteMd
  Add Devicegraph#remove_md
  Move methods to base class Actions::DeleteDevice
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 95.913% when pulling 16bbe8b on fix-use into a3d18df on master.

@yast yast deleted a comment from coveralls Nov 3, 2017
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM

@imobachgs
Copy link
Contributor Author

Thanks!

@imobachgs imobachgs merged commit b078035 into master Nov 3, 2017
@imobachgs imobachgs deleted the fix-use branch November 3, 2017 17:09
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

3 participants