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

many: use new disk.PartitionTableType instead of string #1025

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Nov 7, 2024

This commit makes use of the new `disk.PartitionTableType instead of just using an untyped string (thanks to Achilleas for adding the new type and to Tomáš for suggesting to use it more).


disk: add json marshal/unmarshal to PartitionTableType

This commit adds json marshal/unmarshal to PartitionTableType
The reason is that the otk utils serialize this type
as a JSON. While the json is strictly internal and we make no
gurantees about it is still nicer to serialize as strings because
otherwise the diff in the json looks something like:

@@ -147,7 +147,7 @@ func TestUnmarshalOutput(t *testing.T) {
       "partition-table": {
         "Size": 911,
         "UUID": "",
-        "Type": "gpt",
+        "Type": 2,
         "Partitions": [
           {
             "Start": 0,

and it is also nicer for general debug-ability.

Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

You missed a few (all under cmd/ it seems).

@mvo5 mvo5 force-pushed the use-new-partiton-table-type branch from 2654d8c to 7086d75 Compare November 7, 2024 19:12
@mvo5 mvo5 requested a review from achilleas-k November 8, 2024 08:17
pkg/disk/disk.go Outdated Show resolved Hide resolved
mvo5 added 2 commits November 8, 2024 12:04
This commit adds json marshal/unmarshal to `PartitionTableType`
The reason is that the `otk` utils serialize this type
as a JSON. While the json is strictly internal and we make no
gurantees about it is still nicer to serialize as strings because
otherwise the diff in the json looks something like:
```json
@@ -147,7 +147,7 @@ func TestUnmarshalOutput(t *testing.T) {
       "partition-table": {
         "Size": 911,
         "UUID": "",
-        "Type": "gpt",
+        "Type": 2,
         "Partitions": [
           {
             "Start": 0,
```
and it is also nicer for general debug-ability.
This commit makes use of the new `disk.PartitionTableType instead
of just using an untyped string (thanks to Achilleas for adding
the new type and to Tomáš for suggesting to use it more).
@mvo5 mvo5 force-pushed the use-new-partiton-table-type branch from 7086d75 to 16564da Compare November 8, 2024 11:05
@mvo5 mvo5 requested a review from thozza November 8, 2024 11:05
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

Very nice. Should we do the same for filesystem types?

@achilleas-k
Copy link
Member

Also, can you update the top PR comment/description to avoid confusion in the future?

@mvo5
Copy link
Contributor Author

mvo5 commented Nov 8, 2024

Very nice. Should we do the same for filesystem types?

Yeah, I guess we want this for GetFSType() too (and friends, not sure how many there are)

@mvo5 mvo5 enabled auto-merge November 8, 2024 11:57
@achilleas-k
Copy link
Member

Very nice. Should we do the same for filesystem types?

Yeah, I guess we want this for GetFSType() too (and friends, not sure how many there are)

Fine to do in a separate PR.

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

Amazing 😍

@mvo5 mvo5 added this pull request to the merge queue Nov 8, 2024
Merged via the queue into osbuild:main with commit ad8e175 Nov 8, 2024
19 checks passed
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.

3 participants