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

Drop support for legacy format of ProposalSettings #1239

Merged
merged 3 commits into from Nov 4, 2021

Conversation

ancorgs
Copy link
Contributor

@ancorgs ancorgs commented Oct 28, 2021

Problem

The partitioning proposal can be configured via the control file as explained here: https://github.com/yast/yast-storage-ng/blob/master/doc/old_and_new_proposal.md

During the development of yast-storage-ng, apart from honoring such a configuration, we also added the ability to read proposal configurations written in the old (and way less flexible) format that was understood by the old yast-storage.

No known product/role is using that legacy format. It likely was never used in any released distribution, not even Leap 15.0 or SLE-15.

Still, we have a lot of code just to support that obsolete format.

Solution

Remove all support for the legacy format in ProposalSettings. Moreover, removed all the code that was needed only to support several formats. Now everything is simpler.

Testing

  • Adapted unit tests
  • Tested manually with the proposal_testing client
  • Test coverage has decreased because we now have less code and the removed code was very well covered (even better than the current average). But coverage has not decreased on any of the kept files.

@ancorgs ancorgs force-pushed the proposal_settings_fmt branch 7 times, most recently from dec33f9 to 02be3b8 Compare November 4, 2021 12:44
@coveralls
Copy link

coveralls commented Nov 4, 2021

Coverage Status

Coverage decreased (-0.02%) to 97.707% when pulling 6819734 on ancorgs:proposal_settings_fmt into 10d0479 on yast:master.

@ancorgs ancorgs marked this pull request as ready for review November 4, 2021 13:06
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

@@ -10,7 +10,7 @@
name: /dev/sda1
id: bios_boot
- partition:
size: 15561 MiB (15.20 GiB)
size: 15562 MiB (15.20 GiB)
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glad you asked :-)

Is due to a difference in how the legacy and the NG devices planners used to add the partitions required for booting.

The legacy one performed the calculation taking into account only the root partition as you can see here: https://github.com/yast/yast-storage-ng/blob/381b525/src/lib/y2storage/proposal/devices_planner_strategies/legacy.rb#L56

While the former NG planner (now the only one) takes all existing planned partitions (at that point in time) into account:
https://github.com/yast/yast-storage-ng/blob/2315bb6/src/lib/y2storage/proposal/devices_planner_strategies/ng.rb#L40

Shouldn't the result be the same anyways? Yes, basically it is... except for a tiny detail. The weight of the PlannedPartition for booting is calculated as the sum of the weights of all other known planned partitions. In the case of the legacy strategy that means the weight of the boot-related planned partitions is always equal to the root one. In the case of NG it can be different.

Is that weight really relevant if the boot-related partitions have almost-fixed sizes anyways? It does not affect the size of the boot-related partitions but it can deviate a little bit (only 1 MiB in only one of the existing tests) how the space is distributed between "/" and "/home".

@ancorgs ancorgs merged commit f0c806a into yast:master Nov 4, 2021
@yast-bot
Copy link

yast-bot commented Nov 4, 2021

✔️ Public Jenkins job #388 successfully finished
✔️ Created OBS submit request #929202

@yast-bot
Copy link

yast-bot commented Nov 4, 2021

✔️ Internal Jenkins job #189 successfully finished
✔️ Created IBS submit request #257713

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