From 2edcf1cd1059af962b8bd90c22f0c28708b1159a Mon Sep 17 00:00:00 2001 From: Jan Dubois Date: Fri, 11 Oct 2024 11:10:42 -0700 Subject: [PATCH] =?UTF-8?q?Refactor=20limayaml.Save=20and=20store.SaveYAML?= =?UTF-8?q?=20=E2=86=92=20limayaml.Marshal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `store.SaveYAML` was the only caller of `limayaml.Save`, and was only called from `main.validateAction`. The name `Save` implies writing to a file, so `Marshal` seems like a more logical choice for the name. And since it is marshalling `limayaml` structs it should be in the `limayaml` package and not in `store`. Also move `limayaml.unmarshalYAML` from `load.go` to `marshal.go` for consistency. Signed-off-by: Jan Dubois --- cmd/limactl/validate.go | 3 +- pkg/limayaml/limayaml_test.go | 6 +- pkg/limayaml/load.go | 34 +--------- pkg/limayaml/marshal.go | 62 +++++++++++++++++++ .../{save_test.go => marshal_test.go} | 12 ++-- pkg/limayaml/save.go | 25 -------- pkg/store/store.go | 17 ----- 7 files changed, 77 insertions(+), 82 deletions(-) create mode 100644 pkg/limayaml/marshal.go rename pkg/limayaml/{save_test.go => marshal_test.go} (72%) delete mode 100644 pkg/limayaml/save.go diff --git a/cmd/limactl/validate.go b/cmd/limactl/validate.go index 74e0e3a2927..f62c5db3ee8 100644 --- a/cmd/limactl/validate.go +++ b/cmd/limactl/validate.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/lima-vm/lima/cmd/limactl/guessarg" + "github.com/lima-vm/lima/pkg/limayaml" "github.com/lima-vm/lima/pkg/store" "github.com/spf13/cobra" @@ -38,7 +39,7 @@ func validateAction(cmd *cobra.Command, args []string) error { } logrus.Infof("%q: OK", f) if fill { - b, err := store.SaveYAML(y, len(args) > 1) + b, err := limayaml.Marshal(y, len(args) > 1) if err != nil { return err } diff --git a/pkg/limayaml/limayaml_test.go b/pkg/limayaml/limayaml_test.go index b75e9e0b0b5..c489c0090b6 100644 --- a/pkg/limayaml/limayaml_test.go +++ b/pkg/limayaml/limayaml_test.go @@ -21,7 +21,7 @@ const emptyYAML = "images: []\n" func TestEmptyYAML(t *testing.T) { var y LimaYAML t.Log(dumpJSON(t, y)) - b, err := marshalYAML(y) + b, err := Marshal(&y, false) assert.NilError(t, err) assert.Equal(t, string(b), emptyYAML) } @@ -32,12 +32,12 @@ func TestDefaultYAML(t *testing.T) { bytes, err := os.ReadFile("default.yaml") assert.NilError(t, err) var y LimaYAML - err = unmarshalYAML(bytes, &y, "") + err = Unmarshal(bytes, &y, "") assert.NilError(t, err) y.Images = nil // remove default images y.Mounts = nil // remove default mounts t.Log(dumpJSON(t, y)) - b, err := marshalYAML(y) + b, err := Marshal(&y, false) assert.NilError(t, err) assert.Equal(t, string(b), defaultYAML) } diff --git a/pkg/limayaml/load.go b/pkg/limayaml/load.go index 7aeba343209..511ae2b6307 100644 --- a/pkg/limayaml/load.go +++ b/pkg/limayaml/load.go @@ -6,46 +6,18 @@ import ( "os" "path/filepath" - "github.com/goccy/go-yaml" "github.com/lima-vm/lima/pkg/store/dirnames" "github.com/lima-vm/lima/pkg/store/filenames" - "github.com/lima-vm/lima/pkg/yqutil" "github.com/sirupsen/logrus" ) -func unmarshalDisk(dst *Disk, b []byte) error { - var s string - if err := yaml.Unmarshal(b, &s); err == nil { - *dst = Disk{Name: s} - return nil - } - return yaml.Unmarshal(b, dst) -} - -func unmarshalYAML(data []byte, v interface{}, comment string) error { - if err := yaml.UnmarshalWithOptions(data, v, yaml.DisallowDuplicateKey(), yaml.CustomUnmarshaler[Disk](unmarshalDisk)); err != nil { - return fmt.Errorf("failed to unmarshal YAML (%s): %w", comment, err) - } - // the go-yaml library doesn't catch all markup errors, unfortunately - // make sure to get a "second opinion", using the same library as "yq" - if err := yqutil.ValidateContent(data); err != nil { - return fmt.Errorf("failed to unmarshal YAML (%s): %w", comment, err) - } - if err := yaml.UnmarshalWithOptions(data, v, yaml.Strict(), yaml.CustomUnmarshaler[Disk](unmarshalDisk)); err != nil { - logrus.WithField("comment", comment).WithError(err).Warn("Non-strict YAML is deprecated and will be unsupported in a future version of Lima") - // Non-strict YAML is known to be used by Rancher Desktop: - // https://github.com/rancher-sandbox/rancher-desktop/blob/c7ea7508a0191634adf16f4675f64c73198e8d37/src/backend/lima.ts#L114-L117 - } - return nil -} - // Load loads the yaml and fulfills unspecified fields with the default values. // // Load does not validate. Use Validate for validation. func Load(b []byte, filePath string) (*LimaYAML, error) { var y, d, o LimaYAML - if err := unmarshalYAML(b, &y, fmt.Sprintf("main file %q", filePath)); err != nil { + if err := Unmarshal(b, &y, fmt.Sprintf("main file %q", filePath)); err != nil { return nil, err } configDir, err := dirnames.LimaConfigDir() @@ -57,7 +29,7 @@ func Load(b []byte, filePath string) (*LimaYAML, error) { bytes, err := os.ReadFile(defaultPath) if err == nil { logrus.Debugf("Mixing %q into %q", defaultPath, filePath) - if err := unmarshalYAML(bytes, &d, fmt.Sprintf("default file %q", defaultPath)); err != nil { + if err := Unmarshal(bytes, &d, fmt.Sprintf("default file %q", defaultPath)); err != nil { return nil, err } } else if !errors.Is(err, os.ErrNotExist) { @@ -68,7 +40,7 @@ func Load(b []byte, filePath string) (*LimaYAML, error) { bytes, err = os.ReadFile(overridePath) if err == nil { logrus.Debugf("Mixing %q into %q", overridePath, filePath) - if err := unmarshalYAML(bytes, &o, fmt.Sprintf("override file %q", overridePath)); err != nil { + if err := Unmarshal(bytes, &o, fmt.Sprintf("override file %q", overridePath)); err != nil { return nil, err } } else if !errors.Is(err, os.ErrNotExist) { diff --git a/pkg/limayaml/marshal.go b/pkg/limayaml/marshal.go new file mode 100644 index 00000000000..1e5d3b75475 --- /dev/null +++ b/pkg/limayaml/marshal.go @@ -0,0 +1,62 @@ +package limayaml + +import ( + "fmt" + + "github.com/goccy/go-yaml" + "github.com/lima-vm/lima/pkg/yqutil" + "github.com/sirupsen/logrus" +) + +func marshalString(s string) ([]byte, error) { + if s == "null" || s == "~" { + // work around go-yaml bugs + return []byte("\"" + s + "\""), nil + } + return yaml.Marshal(s) +} + +const ( + documentStart = "---\n" + documentEnd = "...\n" +) + +// Marshal the struct as a YAML document, optionally as a stream. +func Marshal(y *LimaYAML, stream bool) ([]byte, error) { + options := []yaml.EncodeOption{yaml.CustomMarshaler[string](marshalString)} + b, err := yaml.MarshalWithOptions(y, options...) + if err != nil { + return nil, err + } + if stream { + doc := documentStart + string(b) + documentEnd + b = []byte(doc) + } + return b, nil +} + +func unmarshalDisk(dst *Disk, b []byte) error { + var s string + if err := yaml.Unmarshal(b, &s); err == nil { + *dst = Disk{Name: s} + return nil + } + return yaml.Unmarshal(b, dst) +} + +func Unmarshal(data []byte, v interface{}, comment string) error { + if err := yaml.UnmarshalWithOptions(data, v, yaml.DisallowDuplicateKey(), yaml.CustomUnmarshaler[Disk](unmarshalDisk)); err != nil { + return fmt.Errorf("failed to unmarshal YAML (%s): %w", comment, err) + } + // the go-yaml library doesn't catch all markup errors, unfortunately + // make sure to get a "second opinion", using the same library as "yq" + if err := yqutil.ValidateContent(data); err != nil { + return fmt.Errorf("failed to unmarshal YAML (%s): %w", comment, err) + } + if err := yaml.UnmarshalWithOptions(data, v, yaml.Strict(), yaml.CustomUnmarshaler[Disk](unmarshalDisk)); err != nil { + logrus.WithField("comment", comment).WithError(err).Warn("Non-strict YAML is deprecated and will be unsupported in a future version of Lima") + // Non-strict YAML is known to be used by Rancher Desktop: + // https://github.com/rancher-sandbox/rancher-desktop/blob/c7ea7508a0191634adf16f4675f64c73198e8d37/src/backend/lima.ts#L114-L117 + } + return nil +} diff --git a/pkg/limayaml/save_test.go b/pkg/limayaml/marshal_test.go similarity index 72% rename from pkg/limayaml/save_test.go rename to pkg/limayaml/marshal_test.go index 379b3e6209e..dda6d2b5a2b 100644 --- a/pkg/limayaml/save_test.go +++ b/pkg/limayaml/marshal_test.go @@ -7,12 +7,12 @@ import ( "gotest.tools/v3/assert" ) -func TestSaveEmpty(t *testing.T) { - _, err := Save(&LimaYAML{}) +func TestMarshalEmpty(t *testing.T) { + _, err := Marshal(&LimaYAML{}, false) assert.NilError(t, err) } -func TestSaveTilde(t *testing.T) { +func TestMarshalTilde(t *testing.T) { y := LimaYAML{ Mounts: []Mount{ {Location: "~", Writable: ptr.Of(false)}, @@ -20,16 +20,18 @@ func TestSaveTilde(t *testing.T) { {Location: "null"}, }, } - b, err := Save(&y) + b, err := Marshal(&y, true) assert.NilError(t, err) // yaml will load ~ (or null) as null // make sure that it is always quoted - assert.Equal(t, string(b), `images: [] + assert.Equal(t, string(b), `--- +images: [] mounts: - location: "~" writable: false - location: /tmp/lima writable: true - location: "null" +... `) } diff --git a/pkg/limayaml/save.go b/pkg/limayaml/save.go deleted file mode 100644 index 6289344c06a..00000000000 --- a/pkg/limayaml/save.go +++ /dev/null @@ -1,25 +0,0 @@ -package limayaml - -import ( - "github.com/goccy/go-yaml" -) - -func marshalString(s string) ([]byte, error) { - if s == "null" || s == "~" { - // work around go-yaml bugs - return []byte("\"" + s + "\""), nil - } - return yaml.Marshal(s) -} - -func marshalYAML(v interface{}) ([]byte, error) { - options := []yaml.EncodeOption{yaml.CustomMarshaler[string](marshalString)} - return yaml.MarshalWithOptions(v, options...) -} - -// Save saves the yaml. -// -// Save does not fill defaults. Use FillDefaults. -func Save(y *LimaYAML) ([]byte, error) { - return marshalYAML(y) -} diff --git a/pkg/store/store.go b/pkg/store/store.go index 701dd959b02..84885c324c5 100644 --- a/pkg/store/store.go +++ b/pkg/store/store.go @@ -133,20 +133,3 @@ func LoadYAMLByFilePath(filePath string) (*limayaml.LimaYAML, error) { } return y, nil } - -const documentStart = "---\n" - -const documentEnd = "...\n" - -// SaveYAML saves the yaml, optionally as a stream. -func SaveYAML(y *limayaml.LimaYAML, stream bool) ([]byte, error) { - b, err := limayaml.Save(y) - if err != nil { - return nil, err - } - if stream { - doc := documentStart + string(b) + documentEnd - b = []byte(doc) - } - return b, nil -}