search

jaypipes / ghw

Go HardWare discovery/inspection library
Apache License 2.0
1.64k stars 180 forks source link

Fix wrong fs label as partition label #316

Closed Itxaka closed 2 years ago

Itxaka commented 2 years ago

The label entry in the Partition struct should correspond to the partition label, but we were getting the FS label instead.

This patch fixes that by getting the actual partition label for the label value and adding an extra field on the Partition struct to add the fs label, so they are separated and clear on which is which.

Signed-off-by: Itxaka igarcia@suse.com

Itxaka commented 2 years ago

For me it makes sense that the entry at Partition.Label refers to the actual label of the partition itself, if it has. And FsLabel is for the FS that the partition has and can change if the partition is reformatted. The partition label wont change in that case.

Sorry about this :)

Itxaka commented 2 years ago

This breaks the API by changing the underlying value, unfortunately.

Another proposal could be adding 2 new fields, FSLabel and PartLabel and filling those correctly while deprecating the Label field but keeping it like it currently is, so it can be dropped in a couple of versions while we transition.

Itxaka commented 2 years ago

Although one could argue that the API is not really broken, we are fixing the value that it returns because its not really correct and providing another way of getting the older value provided by it.

It would be good to get a look at this asap, as this was recently released (the new Partition.Label entry) so the time to fix it is now, before it can be used generally by other projects cc @jaypipes @fromanirh :)

ffromani commented 2 years ago

Although one could argue that the API is not really broken, we are fixing the value that it returns because its not really correct and providing another way of getting the older value provided by it.

It would be good to get a look at this asap, as this was recently released (the new Partition.Label entry) so the time to fix it is now, before it can be used generally by other projects cc @jaypipes @fromanirh :)

I'll review shortly (ETA worst case 20220509)

ffromani commented 2 years ago

LGTM! thanks a lot!