diff --git a/go.mod b/go.mod index a184bb39d..0c4bb8c16 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/mattn/go-isatty v0.0.19 github.com/posener/complete/v2 v2.1.0 golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63 + golang.org/x/mod v0.12.0 gopkg.in/yaml.v3 v3.0.1 ) diff --git a/go.sum b/go.sum index 420e62639..4783e8963 100644 --- a/go.sum +++ b/go.sum @@ -97,6 +97,8 @@ golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63 h1:m64FZMko/V45gv0bNmrNYoDEq golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63/go.mod h1:0v4NqG35kSWCMzLaMeX+IQrlSnVE/bqGSyC2cz/9Le8= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= +golang.org/x/mod v0.12.0 h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc= +golang.org/x/mod v0.12.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= diff --git a/templates/model/manifest/manifest.go b/templates/model/manifest/manifest.go new file mode 100644 index 000000000..ddf0c06de --- /dev/null +++ b/templates/model/manifest/manifest.go @@ -0,0 +1,114 @@ +// Copyright 2023 The Authors (see AUTHORS file) +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package manifest + +import ( + "errors" + "io" + + "github.com/abcxyz/abc/templates/model" + "gopkg.in/yaml.v3" +) + +// Decode unmarshals the YAML Spec from r. This function exists so we can +// validate the Spec model before providing it to the caller; we don't want the +// caller to forget, and thereby introduce bugs. +// +// If the Spec parses successfully but then fails validation, the spec will be +// returned along with the validation error. +func Decode(r io.Reader) (*Manifest, error) { + out := &Manifest{} + if err := model.DecodeAndValidate(r, out); err != nil { + return nil, err //nolint:wrapcheck + } + return out, nil +} + +// Manifest represents the contents of a manifest file. A manifest file is the +// set of all information that is needed to cleanly upgrade to a new template +// version in the future. +type Manifest struct { + Pos model.ConfigPos `yaml:"-"` + + APIVersion model.String `yaml:"api_version"` + TemplateLocation model.String `yaml:"template_location"` + TemplateDirhash model.String `yaml:"template_dirhash"` + Inputs []*Input `yaml:"inputs"` + OutputHashes []*FileHash `yaml:"output_hashes"` +} + +// UnmarshalYAML implements yaml.Unmarshaler. +func (m *Manifest) UnmarshalYAML(n *yaml.Node) error { + return model.UnmarshalPlain(n, m, &m.Pos) //nolint:wrapcheck +} + +// Validate() implements model.Validator. +func (m *Manifest) Validate() error { + // We don't validate the TemplateDirhash here beyond just verifying that + // it's present; it's validated more later when it's used. + // + // Inputs and OutputHashes can legally be empty. + return errors.Join( + model.IsKnownSchemaVersion(&m.Pos, m.APIVersion, "api_version"), + model.NotZeroModel(&m.Pos, m.TemplateLocation, "template_location"), + model.NotZeroModel(&m.Pos, m.TemplateDirhash, "template_dirhash"), + model.ValidateEach(m.Inputs), + model.ValidateEach(m.OutputHashes), + ) +} + +// Input is a YAML object representing an input value that was provided to the +// template when it was rendered. +type Input struct { + Pos model.ConfigPos + + Name model.String `yaml:"name"` + Value model.String `yaml:"value"` +} + +// UnmarshalYAML implements yaml.Unmarshaler. +func (i *Input) UnmarshalYAML(n *yaml.Node) error { + return model.UnmarshalPlain(n, i, &i.Pos) //nolint:wrapcheck +} + +// Validate() implements model.Validator. +func (i *Input) Validate() error { + return errors.Join( + model.NotZeroModel(&i.Pos, i.Name, "name"), + model.NotZeroModel(&i.Pos, i.Value, "value"), + ) +} + +// FileHash records a checksum of a single file as it was created during +// template rendering. +type FileHash struct { + Pos model.ConfigPos + + File model.String `yaml:"file"` + Hash model.String `yaml:"hash"` // A dirhash-style hash, see https://pkg.go.dev/golang.org/x/mod/sumdb/dirhash +} + +// UnmarshalYAML implements yaml.Unmarshaler. +func (f *FileHash) UnmarshalYAML(n *yaml.Node) error { + return model.UnmarshalPlain(n, f, &f.Pos) //nolint:wrapcheck +} + +// Validate() implements model.Validator. +func (f *FileHash) Validate() error { + return errors.Join( + model.NotZeroModel(&f.Pos, f.File, "file"), + model.NotZeroModel(&f.Pos, f.Hash, "hash"), + ) +} diff --git a/templates/model/manifest/manifest_test.go b/templates/model/manifest/manifest_test.go new file mode 100644 index 000000000..a76ac9dc9 --- /dev/null +++ b/templates/model/manifest/manifest_test.go @@ -0,0 +1,228 @@ +// Copyright 2023 The Authors (see AUTHORS file) +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package manifest + +import ( + "io" + "strings" + "testing" + + "github.com/abcxyz/abc/templates/model" + "github.com/abcxyz/pkg/testutil" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "gopkg.in/yaml.v3" +) + +func TestDecode(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + in string + want *Manifest + wantUnmarshalErr string + wantValidateErr string + }{ + { + name: "simple-success", + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - name: 'my_input_1' + value: 'my_value_1' + - name: 'my_input_2' + value: 'my_value_2' +output_hashes: + - file: 'a/b/c.txt' + hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c' + - file: 'd/e/f.txt' + hash: 'h1:7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730'`, + want: &Manifest{ + APIVersion: model.String{Val: "cli.abcxyz.dev/v1alpha1"}, + TemplateLocation: model.String{Val: "github.com/abcxyz/abc.git//t/rest_server"}, + TemplateDirhash: model.String{Val: "h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03"}, + Inputs: []*Input{ + { + Name: model.String{Val: "my_input_1"}, + Value: model.String{Val: "my_value_1"}, + }, + { + Name: model.String{Val: "my_input_2"}, + Value: model.String{Val: "my_value_2"}, + }, + }, + OutputHashes: []*FileHash{ + { + File: model.String{Val: "a/b/c.txt"}, + Hash: model.String{Val: "h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c"}, + }, + { + File: model.String{Val: "d/e/f.txt"}, + Hash: model.String{Val: "h1:7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730"}, + }, + }, + }, + }, + { + name: "fields-missing", + in: `api_version: "foo"`, + wantValidateErr: `at line 1 column 14: field "api_version" value must be one of [cli.abcxyz.dev/v1alpha1] +at line 1 column 1: field "template_location" is required +at line 1 column 1: field "template_dirhash" is required`, + }, + { + name: "input-missing-name", + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - value: 'my_value_1' +output_hashes: + - file: 'a/b/c.txt' + hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'`, + wantValidateErr: `at line 6 column 5: field "name" is required`, + }, + { + name: "input-missing-value", + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - name: 'my_input_1' +output_hashes: + - file: 'a/b/c.txt' + hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'`, + wantValidateErr: `at line 6 column 5: field "value" is required`, + }, + { + name: "output-hash-missing-file", + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - name: 'my_input_1' + value: 'my_value_1' +output_hashes: + - hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'`, + wantValidateErr: `at line 9 column 5: field "file" is required`, + }, + { + name: "output-hash-missing-file", + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - name: 'my_input_1' + value: 'my_value_1' +output_hashes: + - file: 'a/b/c.txt'`, + wantValidateErr: `at line 9 column 5: field "hash" is required`, + }, + { + name: "no-hashes", // It's rare but legal for a template to have no output files + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - name: 'my_input_1' + value: 'my_value_1' +`, + want: &Manifest{ + APIVersion: model.String{Val: "cli.abcxyz.dev/v1alpha1"}, + TemplateLocation: model.String{Val: "github.com/abcxyz/abc.git//t/rest_server"}, + TemplateDirhash: model.String{Val: "h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03"}, + Inputs: []*Input{ + { + Name: model.String{Val: "my_input_1"}, + Value: model.String{Val: "my_value_1"}, + }, + }, + }, + }, + { + name: "no-inputs", // It's legal for a template to have no inputs + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +output_hashes: + - file: 'a/b/c.txt' + hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'`, + want: &Manifest{ + APIVersion: model.String{Val: "cli.abcxyz.dev/v1alpha1"}, + TemplateLocation: model.String{Val: "github.com/abcxyz/abc.git//t/rest_server"}, + TemplateDirhash: model.String{Val: "h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03"}, + OutputHashes: []*FileHash{ + { + File: model.String{Val: "a/b/c.txt"}, + Hash: model.String{Val: "h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c"}, + }, + }, + }, + }, + { + name: "bad-yaml-syntax", + in: `[[[[[[[`, + wantUnmarshalErr: "did not find expected node content", + }, + } + + for _, tc := range cases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := &Manifest{} + dec := newDecoder(strings.NewReader(tc.in)) + err := dec.Decode(got) + + if diff := testutil.DiffErrString(err, tc.wantUnmarshalErr); diff != "" { + t.Fatal(diff) + } + if err != nil { + return + } + + err = got.Validate() + if diff := testutil.DiffErrString(err, tc.wantValidateErr); diff != "" { + t.Fatal(diff) + } + if err != nil { + return + } + + opt := cmpopts.IgnoreTypes(&model.ConfigPos{}, model.ConfigPos{}) // don't force test authors to assert the line and column numbers + if diff := cmp.Diff(got, tc.want, opt); diff != "" { + t.Errorf("unmarshaling didn't yield expected struct. Diff (-got +want): %s", diff) + } + }) + } +} + +// newDecoder returns a yaml Decoder with the desired options. +func newDecoder(r io.Reader) *yaml.Decoder { + dec := yaml.NewDecoder(r) + dec.KnownFields(true) // Fail if any unexpected fields are seen. Often doesn't work: https://github.com/go-yaml/yaml/issues/460 + return dec +} diff --git a/templates/model/pos.go b/templates/model/pos.go index 7da8eb4b7..a5056ce81 100644 --- a/templates/model/pos.go +++ b/templates/model/pos.go @@ -33,6 +33,10 @@ type ConfigPos struct { Column int } +func (c ConfigPos) IsZero() bool { + return c == ConfigPos{} +} + // YAMLPos constructs a position struct based on a YAML parse cursor. func YAMLPos(n *yaml.Node) *ConfigPos { return &ConfigPos{ @@ -51,7 +55,7 @@ func YAMLPos(n *yaml.Node) *ConfigPos { // Creating an error: c.Errorf("something went wrong doing action %s", action) func (c *ConfigPos) Errorf(fmtStr string, args ...any) error { err := fmt.Errorf(fmtStr, args...) - if c == nil || *c == (ConfigPos{}) { + if c == nil || c.IsZero() { return err } diff --git a/templates/model/spec/spec.go b/templates/model/spec/spec.go index 55f553f22..17632142c 100644 --- a/templates/model/spec/spec.go +++ b/templates/model/spec/spec.go @@ -19,7 +19,6 @@ package spec import ( "errors" - "fmt" "io" "strings" @@ -35,19 +34,17 @@ import ( // If the Spec parses successfully but then fails validation, the spec will be // returned along with the validation error. func Decode(r io.Reader) (*Spec, error) { - dec := newDecoder(r) - var spec Spec - if err := dec.Decode(&spec); err != nil { - return nil, fmt.Errorf("error parsing YAML spec file: %w", err) + // dec := yaml.NewDecoder(r) + // var spec Spec + // if err := dec.Decode(&spec); err != nil { + // return nil, fmt.Errorf("error parsing YAML spec file: %w", err) + // } + // return &spec, spec.Validate() + out := &Spec{} + if err := model.DecodeAndValidate(r, out); err != nil { + return nil, err } - return &spec, spec.Validate() -} - -// newDecoder returns a yaml Decoder with the desired options. -func newDecoder(r io.Reader) *yaml.Decoder { - dec := yaml.NewDecoder(r) - dec.KnownFields(true) // Fail if any unexpected fields are seen. Often doesn't work: https://github.com/go-yaml/yaml/issues/460 - return dec + return out, nil } // Spec represents a parsed spec.yaml file describing a template. diff --git a/templates/model/spec/spec_test.go b/templates/model/spec/spec_test.go index fa2a43d4a..4721c70e5 100644 --- a/templates/model/spec/spec_test.go +++ b/templates/model/spec/spec_test.go @@ -15,6 +15,7 @@ package spec import ( + "io" "strings" "testing" @@ -22,6 +23,7 @@ import ( "github.com/abcxyz/pkg/testutil" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "gopkg.in/yaml.v3" ) func TestSpecUnmarshal(t *testing.T) { @@ -1095,3 +1097,10 @@ params: }) } } + +// newDecoder returns a yaml Decoder with the desired options. +func newDecoder(r io.Reader) *yaml.Decoder { + dec := yaml.NewDecoder(r) + dec.KnownFields(true) // Fail if any unexpected fields are seen. Often doesn't work: https://github.com/go-yaml/yaml/issues/460 + return dec +} diff --git a/templates/model/test/test.go b/templates/model/test/test.go index fd63edae5..ab614c5aa 100644 --- a/templates/model/test/test.go +++ b/templates/model/test/test.go @@ -57,8 +57,15 @@ type Test struct { } // UnmarshalYAML implements yaml.Unmarshaler. -func (i *Test) UnmarshalYAML(n *yaml.Node) error { - return model.UnmarshalPlain(n, i, &i.Pos) //nolint:wrapcheck +func (t *Test) UnmarshalYAML(n *yaml.Node) error { + return model.UnmarshalPlain(n, t, &t.Pos) //nolint:wrapcheck +} + +func (t *Test) Validate() error { + return errors.Join( + model.IsKnownSchemaVersion(&t.Pos, t.APIVersion, "api_version"), + model.ValidateEach(t.Inputs), + ) } // DecodeTest unmarshals the YAML Spec from r. @@ -71,8 +78,8 @@ func DecodeTest(r io.Reader) (*Test, error) { return nil, fmt.Errorf("error parsing test YAML file: %w", err) } - return &test, errors.Join( - model.OneOf(&test.Pos, test.APIVersion, []string{"cli.abcxyz.dev/v1alpha1"}, "apiVersion"), - model.ValidateEach(test.Inputs), - ) + if err := test.Validate(); err != nil { + return nil, err + } + return &test, nil } diff --git a/templates/model/test/test_test.go b/templates/model/test/test_test.go index db70a1900..5f1e5417d 100644 --- a/templates/model/test/test_test.go +++ b/templates/model/test/test_test.go @@ -76,7 +76,7 @@ inputs: in: `inputs: - name: 'person_name' value: 'iron_man'`, - wantErr: `at line 1 column 1: field "apiVersion" value must be one of [cli.abcxyz.dev/v1alpha1]`, + wantErr: `at line 1 column 1: field "api_version" value must be one of [cli.abcxyz.dev/v1alpha1]`, }, } diff --git a/templates/model/unmarshal.go b/templates/model/unmarshal.go index c88ef751b..d64448903 100644 --- a/templates/model/unmarshal.go +++ b/templates/model/unmarshal.go @@ -26,6 +26,8 @@ package model // based on the value of the "action" field. import ( + "fmt" + "io" "reflect" "strings" @@ -86,3 +88,14 @@ func UnmarshalPlain(n *yaml.Node, outPtr any, outPos *ConfigPos, extraYAMLFields *outPos = *YAMLPos(n) return nil } + +// DecodeAndValidate unmarshals the YAML text in the given Reader into the given +// pointer-to-struct, and calls Validate() on it. Returns any unmarshaling error +// or validation error. +func DecodeAndValidate(r io.Reader, outPtr Validator) error { + dec := yaml.NewDecoder(r) + if err := dec.Decode(outPtr); err != nil { + return fmt.Errorf("error parsing YAML spec file: %w", err) + } + return outPtr.Validate() //nolint:wrapcheck +} diff --git a/templates/model/validate.go b/templates/model/validate.go index aeb35d92b..9cecfed93 100644 --- a/templates/model/validate.go +++ b/templates/model/validate.go @@ -70,13 +70,28 @@ func IsValidRegexGroupName(s String, fieldName string) error { } // OneOf returns error if x.Val is not one of the given allowed choices. -func OneOf[T comparable](pos *ConfigPos, x valWithPos[T], allowed []T, fieldName string) error { +func OneOf[T comparable](parentPos *ConfigPos, x valWithPos[T], allowed []T, fieldName string) error { if slices.Contains(allowed, x.Val) { return nil } + pos := x.Pos + if pos == nil || pos.IsZero() { + pos = parentPos + } + return pos.Errorf("field %q value must be one of %v", fieldName, allowed) } +var schemaVersions = []string{"cli.abcxyz.dev/v1alpha1"} + +// IsKnownSchemaVersion TODO +// parentPos is the position of the yaml object that contains the api_version field. We need +// this because if the api_version field is missing from the YAML, then we won't have any +// position information for it. +func IsKnownSchemaVersion(parentPos *ConfigPos, apiVersion String, fieldName string) error { + return OneOf(parentPos, apiVersion, schemaVersions, fieldName) +} + // extrafields returns error if any unexpected fields are seen. The input must // be a mapping/object; anything else will result in an error. This is a // workaround for a bug in KnownFields in the upstream yaml lib