report_hackweek.md

Hackweek project

For this Hackweek two main objectives were proposed: (a) learn, understand and become familiar with proposal code, and (b) improve some classes with a special code smell. Let's see each one.

(a) This was harder than I expected. Proposal implementation has numerous related classes and its correct undestanding took me a while. The first two and a half days was dedicated to this task. Algorithms for deleting partitions, distributing free space or selecting best proposal were studied. Right now I have a good understanding of the code and I feel confortable working on it.

(b) Previous this week, that is, during my first two weeks at YaST teem (yes, this Hackweek came in my third week at SUSE), my workmate Ancor and me talked about different parts of the proposal that could improve. Specially, classes as PlannedVolume and PlannedVolumeList seem to have more responsibilities than they should. In the rest of the Hackweek time I was working on that. Next, implemented improvements are described.

Add proposed classes

One of these code smells is that PlannedVolume class was been used for two different purposes. On one hand, VolumesGenerator populates an instance of PlannedVolumeList with the desired planned volumes. A planned volume represents a certain volume specification, that is, the range of min and max sizes that it should have, etc. This is the input for DeviceGraphGenerator, who has to propose partions and logical volumes to allocate these planned volumes. On the other hand, PlannedVolume is also used for proposing the final partitions or lvs to create and so it has logic for both uses. To avoid that, two new classes have been introduced: ProposedPartition and ProposedLv. Really, there is a third new class, ProposedDevice, that acts as base class.

class ProposedPartition < ProposedDevice
...
end

class ProposedLv < ProposedDevice
...
end

With this approximation, DeviceGraphGenerator takes planned volumes and generates proposed devices. The rest of classes under DeviceGenerator (SpaceMaker, LvmHelper, PartitionCreator, etc) always use proposed devices.

Distribute responsibilities

PlannedVolumeList class has been collecting a lot of responsabilities that it should not have. For example, the extra space distribution among the partitions of a disk or among the lvs of a volume group. This logic is better placed in the classes for creating partitions and lvs. For that, an ExtraSpaceAssigner mixing has been created. This mixing is included by PartitionCreator and LvmHelper classes. PartitionCreator distributes the extra space of a FreeDiskSpace among the partitions associated to this free space. LvmHelper class does the same with the lvs and the extra space of their volume group.

module ExtraSpaceAssigner
  def distribute_space(proposed_devices, ...)
    ...
  end
  ...
end

class PartitionCreator
  include ExtraSpaceAssigner
  ...
end

class LvmHelper
  include ExtraSpaceAssigner
  ...
end

Partitions have to be sorted in most convenient way before creating them. AssignedSpace class represents an association between a FreeDiskSpace and a set of proposed partitions for this space. It seems reasonable that AssignedSpace should be the responsable for sorting its partitions. The logic for that has been moved from PlannedVolumeList and PartitionCreator to AssignedSpace class.

class AssignedSpace

  def initialize(disk_space, proposed_partitions)
    ...
    sort_partitions!
  end

  ...

  def sort_partitions!
    ...
  end
end

Update tests

Of course, tests have been updated to reflect all that changes. Currently, all tests are green.

Pending tasks

One more thing remains for completing all planned improvements. With new introduced classes, PlannedVolumeList can be meaningless, so it should be removed at all and, of course, the current code that uses it should be fixed. Also, the doc of all modified methods has to be updated.