Repository: osbuild/osbuild-composer
Pull request #3783 (2023-11-06 12:49:40)

Blueprints: Fix TOML filesystem `minsize` keyword
Due to an oversight, the toml and json tags for the `MinSize`field had different keywords. This commit fixes this by creating
a `minsize` toml tag and ensuring backwards compatability by checking the old `size` tag.

If both `minsize` & `size` are set in the toml block, the custom unmarshal function validates the input for inconsistencies.

Guides PR: https://github.com/osbuild/guides/pull/152

- [x] adequate testing for the new functionality or fixed issue
- [x] adequate documentation informing people about the change such as
  - [x] submit a PR for the [guides](https://github.com/osbuild/guides) repository if this PR changed any behavior described there: https://www.osbuild.org/guides/

<!--
Thanks for proposing a change to osbuild-composer!

Please don't remove the above check list. These are things that each pull
request must have before it is merged. It helps maintainers to not forget
anything.

If the reason for ticking any of the boxes is ambiguous, please add a short
note explaining why.

In addition, if this pull request fixes a downstream issue, please refer to
test/README.md and add these additional items:

- [ ] 1st commit of any `rhbz#` related PR contains bug reproducer; CI reports FAIL or
- [ ] PR contains automated tests for new functionality and
- [ ] QE has approved reproducer/new tests and
- [ ] Subsequent commits provide bug fixes without modifying the reproducer; CI reports PASS and
- [ ] QE approves this PR; RHBZ status is set to `MODIFIED + Verified=Tested`

Information regarding our GitLab pipeline (Schutzbot):

CI will not be ran automatically if WIP label is applied or the PR is in DRAFT state, instead only
a link will be provided to the pipeline which can then be triggered manually if desired. To run the
CI automatically either switch the PR to ready or apply WIP+test label.

Outside contributors need manual approval from one of the osbuild-composer maintainers.

Schutzbot will only be triggered if all Tests jobs in GitHub workflow succeed.
-->



Patch:
diff --git a/internal/blueprint/filesystem_customizations.go b/internal/blueprint/filesystem_customizations.go
index 02bc36439..3b918506b 100644
--- a/internal/blueprint/filesystem_customizations.go
+++ b/internal/blueprint/filesystem_customizations.go
@@ -9,7 +9,12 @@ import (
 
 type FilesystemCustomization struct {
 	Mountpoint string `json:"mountpoint,omitempty" toml:"mountpoint,omitempty"`
-	MinSize    uint64 `json:"minsize,omitempty" toml:"size,omitempty"`
+	MinSize    uint64 `json:"minsize,omitempty" toml:"minsize,omitempty"`
+
+	// Note: The TOML `size` tag has been deprecated in favor of `minsize`.
+	// we check for it in the TOML unmarshaler and use it as `minsize`.
+	// However due to the TOML marshaler implementation, we can omit adding
+	// a field for this tag and get the benifit of not having to export it.
 }
 
 func (fsc *FilesystemCustomization) UnmarshalTOML(data interface{}) error {
@@ -22,19 +27,60 @@ func (fsc *FilesystemCustomization) UnmarshalTOML(data interface{}) error {
 		return fmt.Errorf("TOML unmarshal: mountpoint must be string, got %v of type %T", d["mountpoint"], d["mountpoint"])
 	}
 
+	var size uint64
+	var minsize uint64
+
+	// `size` is an alias for `minsize. We check for the `size` keyword
+	// for backwards compatibility. We don't export a `Size` field as
+	// we would like to discourage its use.
 	switch d["size"].(type) {
 	case int64:
-		fsc.MinSize = uint64(d["size"].(int64))
+		size = uint64(d["size"].(int64))
 	case string:
-		size, err := common.DataSizeToUint64(d["size"].(string))
+		s, err := common.DataSizeToUint64(d["size"].(string))
 		if err != nil {
 			return fmt.Errorf("TOML unmarshal: size is not valid filesystem size (%w)", err)
 		}
-		fsc.MinSize = size
+		size = s
+	case nil:
+		size = 0
 	default:
 		return fmt.Errorf("TOML unmarshal: size must be integer or string, got %v of type %T", d["size"], d["size"])
 	}
 
+	switch d["minsize"].(type) {
+	case int64:
+		minsize = uint64(d["minsize"].(int64))
+	case string:
+		s, err := common.DataSizeToUint64(d["minsize"].(string))
+		if err != nil {
+			return fmt.Errorf("TOML unmarshal: minsize is not valid filesystem size (%w)", err)
+		}
+		minsize = s
+	case nil:
+		minsize = 0
+	default:
+		return fmt.Errorf("TOML unmarshal: minsize must be integer or string, got %v of type %T", d["minsize"], d["minsize"])
+	}
+
+	if size == 0 && minsize == 0 {
+		return fmt.Errorf("TOML unmarshal: minsize must be greater than 0, got %v", minsize)
+	}
+
+	if size > 0 && minsize == 0 {
+		fsc.MinSize = size
+		return nil
+	}
+
+	if size == 0 && minsize > 0 {
+		fsc.MinSize = minsize
+		return nil
+	}
+
+	if size > 0 && minsize > 0 {
+		return fmt.Errorf("TOML unmarshal: size and minsize cannot both be set (size is an alias for minsize)")
+	}
+
 	return nil
 }
 


Test patch:
diff --git a/internal/blueprint/customizations_test.go b/internal/blueprint/customizations_test.go
index fbbb1135a..58679c0cf 100644
--- a/internal/blueprint/customizations_test.go
+++ b/internal/blueprint/customizations_test.go
@@ -311,68 +311,6 @@ func TestNilGetTimezoneSettings(t *testing.T) {
 	assert.Nil(t, retNTPServers)
 }
 
-func TestGetFilesystems(t *testing.T) {
-
-	expectedFilesystems := []FilesystemCustomization{
-		{
-			MinSize:    1024,
-			Mountpoint: "/",
-		},
-	}
-
-	TestCustomizations := Customizations{
-		Filesystem: expectedFilesystems,
-	}
-
-	retFilesystems := TestCustomizations.GetFilesystems()
-
-	assert.ElementsMatch(t, expectedFilesystems, retFilesystems)
-}
-
-func TestGetFilesystemsMinSize(t *testing.T) {
-
-	expectedFilesystems := []FilesystemCustomization{
-		{
-			MinSize:    1024,
-			Mountpoint: "/",
-		},
-		{
-			MinSize:    4096,
-			Mountpoint: "/var",
-		},
-	}
-
-	TestCustomizations := Customizations{
-		Filesystem: expectedFilesystems,
-	}
-
-	retFilesystemsSize := TestCustomizations.GetFilesystemsMinSize()
-
-	assert.EqualValues(t, uint64(5120), retFilesystemsSize)
-}
-
-func TestGetFilesystemsMinSizeNonSectorSize(t *testing.T) {
-
-	expectedFilesystems := []FilesystemCustomization{
-		{
-			MinSize:    1025,
-			Mountpoint: "/",
-		},
-		{
-			MinSize:    4097,
-			Mountpoint: "/var",
-		},
-	}
-
-	TestCustomizations := Customizations{
-		Filesystem: expectedFilesystems,
-	}
-
-	retFilesystemsSize := TestCustomizations.GetFilesystemsMinSize()
-
-	assert.EqualValues(t, uint64(5632), retFilesystemsSize)
-}
-
 func TestGetOpenSCAPConfig(t *testing.T) {
 
 	expectedOscap := OpenSCAPCustomization{
diff --git a/internal/blueprint/filesystem_customizations_test.go b/internal/blueprint/filesystem_customizations_test.go
new file mode 100644
index 000000000..5511ac672
--- /dev/null
+++ b/internal/blueprint/filesystem_customizations_test.go
@@ -0,0 +1,159 @@
+package blueprint
+
+import (
+	"testing"
+
+	"github.com/BurntSushi/toml"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetFilesystems(t *testing.T) {
+
+	expectedFilesystems := []FilesystemCustomization{
+		{
+			MinSize:    1024,
+			Mountpoint: "/",
+		},
+	}
+
+	TestCustomizations := Customizations{
+		Filesystem: expectedFilesystems,
+	}
+
+	retFilesystems := TestCustomizations.GetFilesystems()
+
+	assert.ElementsMatch(t, expectedFilesystems, retFilesystems)
+}
+
+func TestGetFilesystemsMinSize(t *testing.T) {
+
+	expectedFilesystems := []FilesystemCustomization{
+		{
+			MinSize:    1024,
+			Mountpoint: "/",
+		},
+		{
+			MinSize:    4096,
+			Mountpoint: "/var",
+		},
+	}
+
+	TestCustomizations := Customizations{
+		Filesystem: expectedFilesystems,
+	}
+
+	retFilesystemsSize := TestCustomizations.GetFilesystemsMinSize()
+
+	assert.EqualValues(t, uint64(5120), retFilesystemsSize)
+}
+
+func TestGetFilesystemsMinSizeNonSectorSize(t *testing.T) {
+
+	expectedFilesystems := []FilesystemCustomization{
+		{
+			MinSize:    1025,
+			Mountpoint: "/",
+		},
+		{
+			MinSize:    4097,
+			Mountpoint: "/var",
+		},
+	}
+
+	TestCustomizations := Customizations{
+		Filesystem: expectedFilesystems,
+	}
+
+	retFilesystemsSize := TestCustomizations.GetFilesystemsMinSize()
+
+	assert.EqualValues(t, uint64(5632), retFilesystemsSize)
+}
+
+func TestGetFilesystemsMinSizeTOML(t *testing.T) {
+
+	tests := []struct {
+		Name  string
+		TOML  string
+		Want  []FilesystemCustomization
+		Error bool
+	}{
+		{
+			Name: "size set, no minsize",
+			TOML: `
+[[customizations.filesystem]]
+mountpoint = "/var"
+size = 1024
+			`,
+			Want:  []FilesystemCustomization{{MinSize: 1024, Mountpoint: "/var"}},
+			Error: false,
+		},
+		{
+			Name: "size set (string), no minsize",
+			TOML: `
+[[customizations.filesystem]]
+mountpoint = "/var"
+size = "1KiB"
+			`,
+			Want:  []FilesystemCustomization{{MinSize: 1024, Mountpoint: "/var"}},
+			Error: false,
+		},
+		{
+			Name: "minsize set, no size",
+			TOML: `
+[[customizations.filesystem]]
+mountpoint = "/var"
+minsize = 1024
+			`,
+			Want:  []FilesystemCustomization{{MinSize: 1024, Mountpoint: "/var"}},
+			Error: false,
+		},
+		{
+			Name: "minsize set (string), no size",
+			TOML: `
+[[customizations.filesystem]]
+mountpoint = "/var"
+minsize = "1KiB"
+			`,
+			Want:  []FilesystemCustomization{{MinSize: 1024, Mountpoint: "/var"}},
+			Error: false,
+		},
+		{
+			Name: "size and minsize set",
+			TOML: `
+[[customizations.filesystem]]
+mountpoint = "/var"
+size = 1024
+minsize = 1024
+			`,
+			Want:  []FilesystemCustomization{},
+			Error: true,
+		},
+		{
+			Name: "size and minsize not set",
+			TOML: `
+[[customizations.filesystem]]
+mountpoint = "/var"
+			`,
+			Want:  []FilesystemCustomization{},
+			Error: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.Name, func(t *testing.T) {
+
+			var blueprint Blueprint
+			err := toml.Unmarshal([]byte(tt.TOML), &blueprint)
+
+			if tt.Error {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				assert.NotNil(t, blueprint.Customizations)
+				assert.Equal(t, tt.Want, blueprint.Customizations.Filesystem)
+			}
+		})
+
+	}
+
+}