Add storage checks #468

joseivanlopez · 2017-12-15T12:25:04Z

PBI: https://trello.com/c/PxAc8OlA/766-5-expert-partitioner-add-sanity-checks

coveralls · 2018-01-15T16:43:12Z

Coverage increased (+0.05%) to 96.28% when pulling f149102 on joseivanlopez:setup_checker into 1831961 on yast:master.

ancorgs · 2018-01-15T17:06:45Z

package/yast2-storage-ng.spec

@@ -16,7 +16,7 @@
 #

 Name:		yast2-storage-ng
-Version:        4.0.68
+Version:        4.0.70


Why are you jumping two numbers here?

ancorgs · 2018-01-15T17:07:10Z

package/yast2-storage-ng.changes

@@ -6,6 +14,7 @@ Mon Jan 15 12:51:01 UTC 2018 - ancor@suse.com
  alignment types, in addition to the optimal one.
 - Added a (temporary) workaround to a possible bug in libstorage-ng
  regarding alignment.
+- 4.0.69


This was not intended to introduce a version bump, no need to "correct" that

ancorgs · 2018-01-15T17:09:55Z

package/yast2-storage-ng.changes

+
+- Added sanity checks for partitioning setup.
+- Partitioner: setup issues are shown to the user before continue.
+  Mandatory product volumes are required according to control file.


I miss the storage-ng fate entry and also mentioning the several bug reports we have about the missing checks.

ancorgs · 2018-01-15T17:11:37Z

test/support/proposed_partitions_examples.rb

@@ -69,7 +69,6 @@
    it "requires it to be between 1 and 8MiB, despite the alignment" do
      expect(grub_part.min).to eq 1.MiB
      expect(grub_part.max).to eq 8.MiB
-      expect(grub_part.align).to eq :keep_size


Let's remove also the "despite the alignment" part from the textual description

ancorgs · 2018-01-15T17:11:49Z

test/support/proposed_partitions_examples.rb

@@ -79,7 +78,6 @@
    it "requires it to be between 256KiB and 8MiB, despite the alignment" do
      expect(grub_part.min).to eq 256.KiB
      expect(grub_part.max).to eq 8.MiB
-      expect(grub_part.align).to eq :keep_size


Same here for "despite the alignment"

ancorgs · 2018-01-15T17:12:04Z

test/support/proposed_partitions_examples.rb

@@ -138,7 +136,6 @@
    it "requires it to be between 1MiB and 8MiB, despite the alignment" do
      expect(prep_part.min).to eq 1.MiB
      expect(prep_part.max).to eq 8.MiB
-      expect(prep_part.align).to eq :keep_size


Same here for "despite the alignment"

ancorgs · 2018-01-15T17:12:11Z

test/support/proposed_partitions_examples.rb

@@ -148,7 +145,6 @@
    it "requires it to be between 256KiB and 8MiB, despite the alignment" do
      expect(prep_part.min).to eq 256.KiB
      expect(prep_part.max).to eq 8.MiB
-      expect(prep_part.align).to eq :keep_size


Same here for "despite the alignment"

ancorgs · 2018-01-15T17:17:04Z

src/lib/y2storage/md.rb

+    # BIOS RAIDs. Class MdContainer is also derived from Md, but objects of this
+    # class are not be considered neither Software nor BIOS RAIDs.
+    # To properly exclude MdContainer as :raid device, here only Software RAIDs
+    # are considered to be raid type.


NP: Wouldn't be too much to ask to have this explanation right on top of the types << :raid if software_defined? line instead of the heading of the method?

ancorgs · 2018-01-15T17:19:07Z

src/lib/y2storage/planned/md.rb

@@ -105,7 +105,7 @@ def self.to_string_attrs
      def volume_match_values
        {
          mount_point:  mount_point,
-          size:         DiskSize.zero,
+          size:         nil,


Why don't we want to check the size of the Md devices? Doesn't Md#size return a sensible value just like e.g. Partition#size?

It is a Planned::Md and it has not #size.

ancorgs · 2018-01-15T17:23:04Z

src/lib/y2storage/filesystems/type.rb

+      #   Session15694/SHARE_Bootloader_Ihno_PittsPPT_0.09.pdf
+      #
+      # @return [Array<Filesystems::Type>]
+      def self.zipl_filesystems


It should be mentioned that the first one is the preferred one (i.e. used by the guided setup)

ancorgs · 2018-01-15T17:25:32Z

src/lib/y2storage/boot_requirements_strategies/zipl.rb

-        error_message = _("Current boot disk cannot be used for booting")
+        error_message = _(
+          "Looks like the system is going to be installed on a FBA " \
+          "or LDL device. Booting from such device is not supported"


Missing period at the end?

ancorgs · 2018-01-15T17:28:07Z

src/lib/y2partitioner/widgets/partition_description.rb

@@ -60,7 +60,7 @@ def device_attributes_list
          device_udev_by_path.join(Yast::HTML.Newline),
          device_udev_by_id.join(Yast::HTML.Newline),
          # TRANSLATORS: acronym for Filesystem Identifier
-          format(_("FS ID: %s"), "TODO")
+          format(_("Partition ID: %s"), "TODO")


Please update the comment for the translators

coveralls · 2018-01-15T17:54:18Z

Coverage increased (+0.05%) to 96.28% when pulling 6f23a5e on joseivanlopez:setup_checker into 1831961 on yast:master.

ancorgs · 2018-01-16T07:25:57Z

LGTM, let's go for it!

joseivanlopez force-pushed the setup_checker branch from f3acfd9 to ad47165 Compare December 15, 2017 15:08

yast deleted a comment from coveralls Dec 18, 2017

joseivanlopez force-pushed the setup_checker branch from 431a760 to 7af69c8 Compare December 19, 2017 10:00

yast deleted a comment from coveralls Dec 19, 2017

joseivanlopez force-pushed the setup_checker branch from c8350be to 3f61aed Compare December 19, 2017 10:52

yast deleted a comment from coveralls Dec 19, 2017

joseivanlopez force-pushed the setup_checker branch 5 times, most recently from 17a5632 to 1d4d9d7 Compare January 10, 2018 15:57

yast deleted a comment from coveralls Jan 10, 2018

joseivanlopez force-pushed the setup_checker branch 3 times, most recently from 7defe9b to 170624a Compare January 12, 2018 17:25

yast deleted a comment from coveralls Jan 12, 2018

joseivanlopez added 6 commits January 15, 2018 16:24

Remove align from planned partitions

26e3465

Method #to_html always returns a string

690ff06

Several fixes

53f5af4

Allow more filesystems for zipl

6425d89

Improve wording

c7133b7

Rename filesystem id to partition id

645f734

joseivanlopez force-pushed the setup_checker branch from 68cc6a8 to 645f734 Compare January 15, 2018 16:25

Silly fixes

f149102

joseivanlopez force-pushed the setup_checker branch from 09e1cd7 to f149102 Compare January 15, 2018 16:36

yast deleted a comment from coveralls Jan 15, 2018

ancorgs reviewed Jan 15, 2018

View reviewed changes

joseivanlopez added 2 commits January 15, 2018 17:45

Improve changelog and fix version number

d716a7e

Several fixes

6f23a5e

ancorgs approved these changes Jan 16, 2018

View reviewed changes

joseivanlopez merged commit 52089a7 into yast:master Jan 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add storage checks #468

Add storage checks #468

joseivanlopez commented Dec 15, 2017

coveralls commented Jan 15, 2018

ancorgs Jan 15, 2018

joseivanlopez Jan 15, 2018

ancorgs Jan 15, 2018

ancorgs Jan 15, 2018

ancorgs Jan 15, 2018

ancorgs Jan 15, 2018

ancorgs Jan 15, 2018

ancorgs Jan 15, 2018

ancorgs Jan 15, 2018

ancorgs Jan 15, 2018

joseivanlopez Jan 15, 2018

ancorgs Jan 15, 2018

ancorgs Jan 15, 2018 •

edited

ancorgs Jan 15, 2018

coveralls commented Jan 15, 2018

ancorgs commented Jan 16, 2018

Add storage checks #468

Add storage checks #468

Conversation

joseivanlopez commented Dec 15, 2017

coveralls commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ancorgs Jan 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 15, 2018

ancorgs commented Jan 16, 2018

ancorgs Jan 15, 2018 •

edited