fcos/v1_6_exp: warn if config attempts to reuse partition by label

[Adam0Brien]

Fix : #377

[bgilbert]

Thanks for working on this!

[bgilbert]

FYI, #434 will land first, so you'll eventually have to rebase this onto fcos/v1_6_exp.

[Adam0Brien]

Marking this as a Draft PR as i'm not too familiar with butane yet.

[Adam0Brien]

Test is working fine now : fails when a partition has the number 0 and wipe_tables is set to false

[bgilbert]

Have you manually tested with the config from #377?

[Adam0Brien]

I haven't only the test that is in translate_test.go and i'm not entirely sure how i would go about testing manually

[bgilbert]

You should be able to just run Butane and pass it the config, and see whether it outputs the warning.

[Adam0Brien]

this is the output i got :

adamobrien@fedora:~/coreos/coreos-butane/butane(p-tables-tests⚡) » butane config.yaml 2 ↵
{"ignition":{"version":"3.3.0"},"storage":{"disks":[{"device":"/dev/sdb","partitions":[{"label":"foo"}],"wipeTable":false}]}}

looks okay

[bgilbert]

There's no warning there.

[Adam0Brien]

There's not supposed to be a warning is there?

[bgilbert]

wipeTable is set to false and there's a partition with number 0, so yes.

[Adam0Brien]

my bad i thought there shouldn't be one taking a look now!

[Adam0Brien]

okay so after running this config on a local build

version: 1.4.0
storage:
  disks:
    - device: /dev/sdb
      wipe_table: false
      partitions:
        - label: foo
        - number: 1

generates a .ign fine

but if i was to change the number from 1 -> 0 then i get the following error
error at $.storage.disks.0.partitions.1, line 9 col 11: a partition number >= 1 or a label must be specified Error translating config: config generated was invalid

[bgilbert]

You're declaring two separate partitions but I think you're intending to declare one. You'll probably want to drop the - from the last line.

[Adam0Brien]

my bad didn't know that made a new partition. tested it without the - and i didn't get an error...

[Adam0Brien]

At the moment if i use this config butane will return the error message "Partition number cannot be zero.% "
works as intended but id love some feedback

version: 1.5.0-experimental
storage:
  disks:
    - device: /dev/sdb
      wipe_table: false
      partitions:
        - label: foo```

[bgilbert]

The PR title and commit message still need details.

[bgilbert]

Tests are failing.

[bgilbert]

For the commit message, I wrote this text:

fcos/v1_6_exp: warn if config attempts to reuse partition by label

Ignition matches existing partitions by number, not by label.  When
reusing an existing partition table, if the config specifies a label
and leaves the number at the default of 0, Ignition will try to add a
new partition rather than reusing any existing one.  That was probably
not the user's intention, so warn in this case.

However, the boot disk is a common exception, since it has a fresh
partition table on every run.  On FCOS we can detect the alias
/dev/disk/by-id/coreos-boot-disk and not warn, but that alias was added
recently and older configs won't use it.  On other variants we don't
have any way to detect the boot disk.  Therefore, don't implement the
warning in base and don't make it retroactive to existing stable specs;
only add it to the FCOS experimental spec.

Suggestions/edits welcome.

[bgilbert]

Looks good overall! A couple small fixes and then this should be ready.

[bgilbert]

LGTM. Please squash your commits and I'll approve.