ubiquity

Merge ~jibel/ubiquity:zfs_fixes into ubiquity:master

  1. Git
  2. lp:~jibel/ubiquity
  3. zfs_fixes
  4. Merge into master
Proposed by Jean-Baptiste Lallement
Status: Merged
Merged at revision: 3d62cb23e755f1afbb5f8027edf441b23799eb8c
Proposed branch: ~jibel/ubiquity:zfs_fixes
Merge into: ubiquity:master
Diff against target: 461 lines (+240/-90)
4 files modified
debian/changelog (+13/-3)
debian/ubiquity.templates (+5/-0)
scripts/zsys-setup (+181/-86)
ubiquity/plugins/ubi-partman.py (+41/-1)
Reviewer Review Type Date Requested Status
Didier Roche-Tolomelli (community) Approve
Review via email: mp+375238@code.staging.launchpad.net

Commit message

Various fixes from experiment:
  - Use version 5000 for bpool with some features disabled to stay compatible with grub and prevent users from upgrading and breaking their systems.
  - The partman confirmation dialog now displays the final layout of the partition that will be created instead of the true but confusing message from partman telling that an ext4 partition will be created (LP: #1847719)
  - Calculate the size of bpool to be 500M < 5% partition size < 2G
  - Always create an ESP and moved grub to the ESP.

To post a comment you must log in.
Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

I didn't test yet as we are going to have some iterations. Here are mostly nitpick, good work :)

Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

Thanks fir the review. Comments addressed.

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

- one more comment on the new changes
- still 2 pending on previous diff

Revision history for this message
Jean-Baptiste Lallement (jibel) :
Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

LGTM! I didn't give it a try yet, but I didn't spot anything more. Just 2 notes:
- Is having predictable names in /tmp is a security issue? I remember we removed from flag filenames in the past due to this.
- Did you have a look at our latest exchanges with Richard (the ones before the 19.10 release) to ensure we address all of his remarks?

Ack (pending testing)

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Did a test-run without UEFI enabled, and working well for me.

Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

I reviewed the comments from Richard in the spec and latest email exchanges and apparently they've all been addressed.

About the temporary directory in /tmp we could pass an additional argument to the script, but it's called from completely different scopes: from ubiquity itself (ubi-partman and gtk_ui) and from a subprocess, it would add lot of complexity for share the name of the temporary directory between these scopes. Passing it in the environment wouldn't improve the security either. At best we could umask the directory so it can only be accessed by root.

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

+1 for me then :)

review: Approve

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches