From 2d9f5a978aa282619d2965fc5db164b082665e79 Mon Sep 17 00:00:00 2001 From: David Gannon <19214156+dgannon991@users.noreply.github.com> Date: Sun, 1 Sep 2024 19:59:44 +0100 Subject: [PATCH 1/3] Added parsing step and tests Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com> --- pkg/manifest/manifest.go | 48 +++++++++++++++++-- pkg/manifest/manifest_test.go | 38 +++++++++++++++ .../testdata/mixin-with-empty-version.yaml | 6 +++ .../testdata/mixin-with-invalid-version.yaml | 6 +++ .../testdata/mixin-with-versions.yaml | 6 +++ 5 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 pkg/manifest/testdata/mixin-with-empty-version.yaml create mode 100644 pkg/manifest/testdata/mixin-with-invalid-version.yaml create mode 100644 pkg/manifest/testdata/mixin-with-versions.yaml diff --git a/pkg/manifest/manifest.go b/pkg/manifest/manifest.go index 5c9b63c7a..23e11a641 100644 --- a/pkg/manifest/manifest.go +++ b/pkg/manifest/manifest.go @@ -640,8 +640,30 @@ func (l Location) IsEmpty() bool { } type MixinDeclaration struct { - Name string - Config interface{} + Name string + Version *semver.Constraints + Config interface{} +} + +func extractVersionFromName(name string) (string, *semver.Constraints, error) { + parts := strings.Split(name, "@") + + // if there isn't a version in the name, just stop! + if len(parts) == 1 { + return name, nil, nil + } + + // if we somehow got more parts than expected! + if len(parts) != 2 { + return "", nil, fmt.Errorf("expected name@version, got: %s", name) + } + + version, err := semver.NewConstraint(parts[1]) + if err != nil { + return "", nil, err + } + + return parts[0], version, nil } // UnmarshalYAML allows mixin declarations to either be a normal list of strings @@ -652,12 +674,25 @@ type MixinDeclaration struct { // - az: // extensions: // - iot +// +// for each type, we can optionally support a version number in the name field +// mixins: +// - exec@2.1.1 +// or +// - az@2.1.1 +// extensions: +// - iot func (m *MixinDeclaration) UnmarshalYAML(unmarshal func(interface{}) error) error { // First try to just read the mixin name var mixinNameOnly string err := unmarshal(&mixinNameOnly) if err == nil { - m.Name = mixinNameOnly + name, version, err := extractVersionFromName(mixinNameOnly) + if err != nil { + return fmt.Errorf("invalid mixin name/version: %w", err) + } + m.Name = name + m.Version = version m.Config = nil return nil } @@ -676,7 +711,12 @@ func (m *MixinDeclaration) UnmarshalYAML(unmarshal func(interface{}) error) erro } for mixinName, config := range mixinWithConfig { - m.Name = mixinName + name, version, err := extractVersionFromName(mixinName) + if err != nil { + return fmt.Errorf("invalid mixin name/version: %w", err) + } + m.Name = name + m.Version = version m.Config = config break // There is only one mixin anyway but break for clarity } diff --git a/pkg/manifest/manifest_test.go b/pkg/manifest/manifest_test.go index 79285d476..5d8ebacda 100644 --- a/pkg/manifest/manifest_test.go +++ b/pkg/manifest/manifest_test.go @@ -12,6 +12,7 @@ import ( "get.porter.sh/porter/pkg/portercontext" "get.porter.sh/porter/pkg/schema" "get.porter.sh/porter/pkg/yaml" + "github.com/Masterminds/semver/v3" "github.com/cnabio/cnab-go/bundle/definition" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -589,6 +590,43 @@ func TestMixinDeclaration_UnmarshalYAML_Invalid(t *testing.T) { assert.Contains(t, err.Error(), "mixin declaration contained more than one mixin") } +func TestMixinDeclaration_UnmarshalYAML_Versions(t *testing.T) { + cxt := portercontext.NewTestContext(t) + cxt.AddTestFile("testdata/mixin-with-versions.yaml", config.Name) + m, err := ReadManifest(cxt.Context, config.Name, config.NewTestConfig(t).Config) + + execVersion, _ := semver.NewConstraint("1") + axVersion, _ := semver.NewConstraint("1.1.X") + terraformVersoin, _ := semver.NewConstraint(">=2") + + require.NoError(t, err) + assert.Len(t, m.Mixins, 3, "expected 3 mixins") + assert.Equal(t, "exec", m.Mixins[0].Name) + assert.Equal(t, "az", m.Mixins[1].Name) + assert.Equal(t, "terraform", m.Mixins[2].Name) + assert.Equal(t, execVersion, m.Mixins[0].Version) + assert.Equal(t, axVersion, m.Mixins[1].Version) + assert.Equal(t, terraformVersoin, m.Mixins[2].Version) +} + +func TestMixinDeclaration_UnmarshalYAML_Versions_Empty(t *testing.T) { + cxt := portercontext.NewTestContext(t) + cxt.AddTestFile("testdata/mixin-with-empty-version.yaml", config.Name) + _, err := ReadManifest(cxt.Context, config.Name, config.NewTestConfig(t).Config) + + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid mixin name/version: improper constraint:") +} + +func TestMixinDeclaration_UnmarshalYAML_Versions_Invalid(t *testing.T) { + cxt := portercontext.NewTestContext(t) + cxt.AddTestFile("testdata/mixin-with-invalid-version.yaml", config.Name) + _, err := ReadManifest(cxt.Context, config.Name, config.NewTestConfig(t).Config) + + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid mixin name/version: expected name@version, got: az@this@that") +} + func TestCredentialsDefinition_UnmarshalYAML(t *testing.T) { assertAllCredentialsRequired := func(t *testing.T, creds CredentialDefinitions) { for _, cred := range creds { diff --git a/pkg/manifest/testdata/mixin-with-empty-version.yaml b/pkg/manifest/testdata/mixin-with-empty-version.yaml new file mode 100644 index 000000000..0dd25aa63 --- /dev/null +++ b/pkg/manifest/testdata/mixin-with-empty-version.yaml @@ -0,0 +1,6 @@ +mixins: + - exec@ + - az: + extensions: + - iot + - terraform diff --git a/pkg/manifest/testdata/mixin-with-invalid-version.yaml b/pkg/manifest/testdata/mixin-with-invalid-version.yaml new file mode 100644 index 000000000..7deab42a4 --- /dev/null +++ b/pkg/manifest/testdata/mixin-with-invalid-version.yaml @@ -0,0 +1,6 @@ +mixins: + - exec + - az@this@that: + extensions: + - iot + - terraform diff --git a/pkg/manifest/testdata/mixin-with-versions.yaml b/pkg/manifest/testdata/mixin-with-versions.yaml new file mode 100644 index 000000000..1017c0977 --- /dev/null +++ b/pkg/manifest/testdata/mixin-with-versions.yaml @@ -0,0 +1,6 @@ +mixins: + - exec@1 + - az@1.1.X: + extensions: + - iot + - terraform@>=2 From 81acfa7ebbe423652c755646c1c15041382fabd3 Mon Sep 17 00:00:00 2001 From: David Gannon <19214156+dgannon991@users.noreply.github.com> Date: Fri, 6 Sep 2024 23:46:14 +0100 Subject: [PATCH 2/3] Added linting step and tests Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com> --- pkg/linter/linter.go | 38 +++++++++++++++ pkg/linter/linter_test.go | 100 ++++++++++++++++++++++++++++++++++++++ pkg/mixin/helpers.go | 11 +++-- 3 files changed, 146 insertions(+), 3 deletions(-) diff --git a/pkg/linter/linter.go b/pkg/linter/linter.go index be1f03415..79ecf6b01 100644 --- a/pkg/linter/linter.go +++ b/pkg/linter/linter.go @@ -13,6 +13,7 @@ import ( "get.porter.sh/porter/pkg/portercontext" "get.porter.sh/porter/pkg/tracing" "get.porter.sh/porter/pkg/yaml" + "github.com/Masterminds/semver/v3" "github.com/dustin/go-humanize" ) @@ -264,9 +265,46 @@ func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest, config *config. results = append(results, r...) } + span.Debug("Getting versions for each mixin used in the manifest...") + err = l.validateVersionNumberConstraints(ctx, m) + if err != nil { + return nil, span.Error(err) + } + return results, nil } +func (l *Linter) validateVersionNumberConstraints(ctx context.Context, m *manifest.Manifest) error { + for _, mixin := range m.Mixins { + if mixin.Version != nil { + installedMeta, err := l.Mixins.GetMetadata(ctx, mixin.Name) + if err != nil { + return fmt.Errorf("unable to get metadata from mixin %s: %w", mixin.Name, err) + } + installedVersion := installedMeta.GetVersionInfo().Version + + err = validateSemverConstraint(mixin.Name, installedVersion, mixin.Version) + if err != nil { + return err + } + } + } + + return nil +} + +func validateSemverConstraint(name string, installedVersion string, versionConstraint *semver.Constraints) error { + v, err := semver.NewVersion(installedVersion) + if err != nil { + return fmt.Errorf("invalid version number from mixin %s: %s. %w", name, installedVersion, err) + } + + if !versionConstraint.Check(v) { + return fmt.Errorf("mixin %s is installed at version %s but your bundle requires version %s", name, installedVersion, versionConstraint) + } + return nil +} + func validateParamsAppliesToAction(m *manifest.Manifest, steps manifest.Steps, tmplParams manifest.ParameterDefinitions, actionName string, config *config.Config) (Results, error) { var results Results for stepNumber, step := range steps { diff --git a/pkg/linter/linter_test.go b/pkg/linter/linter_test.go index 895a94afa..e0b27e0b1 100644 --- a/pkg/linter/linter_test.go +++ b/pkg/linter/linter_test.go @@ -8,7 +8,10 @@ import ( "get.porter.sh/porter/pkg/config" "get.porter.sh/porter/pkg/manifest" "get.porter.sh/porter/pkg/mixin" + "get.porter.sh/porter/pkg/pkgmgmt" "get.porter.sh/porter/pkg/portercontext" + "get.porter.sh/porter/tests" + "github.com/Masterminds/semver/v3" "github.com/stretchr/testify/require" ) @@ -363,3 +366,100 @@ func TestLinter_DependencyMultipleTimes(t *testing.T) { require.Len(t, results, 0, "linter should have returned 0 result") }) } + +func TestLinter_Lint_MissingMixin(t *testing.T) { + cxt := portercontext.NewTestContext(t) + mixins := mixin.NewTestMixinProvider() + l := New(cxt.Context, mixins) + testConfig := config.NewTestConfig(t).Config + + mixinName := "made-up-mixin-that-is-not-installed" + + m := &manifest.Manifest{ + Mixins: []manifest.MixinDeclaration{ + { + Name: mixinName, + }, + }, + } + + mixins.RunAssertions = append(mixins.RunAssertions, func(mixinCxt *portercontext.Context, mixinName string, commandOpts pkgmgmt.CommandOptions) error { + return fmt.Errorf("%s not installed", mixinName) + }) + + _, err := l.Lint(context.Background(), m, testConfig) + require.Error(t, err, "Linting should return an error") + tests.RequireOutputContains(t, err.Error(), fmt.Sprintf("%s is not currently installed", mixinName)) +} + +func TestLinter_Lint_MixinVersions(t *testing.T) { + cxt := portercontext.NewTestContext(t) + mixinProvider := mixin.NewTestMixinProvider() + l := New(cxt.Context, mixinProvider) + testConfig := config.NewTestConfig(t).Config + + exampleMixinVersion := mixin.ExampleMixinSemver.String() + + // build up some test semvers + patchDifferenceSemver := fmt.Sprintf("%d.%d.%d", mixin.ExampleMixinSemver.Major(), mixin.ExampleMixinSemver.Minor(), mixin.ExampleMixinSemver.Patch()+1) + anyPatchAccepted := fmt.Sprintf("%d.%d.x", mixin.ExampleMixinSemver.Major(), mixin.ExampleMixinSemver.Minor()) + lessThanNextMajor := fmt.Sprintf("<%d.%d", mixin.ExampleMixinSemver.Major()+1, mixin.ExampleMixinSemver.Minor()) + + exampleMixinVersionConstraint, _ := semver.NewConstraint(exampleMixinVersion) + patchDifferenceSemverConstraint, _ := semver.NewConstraint(patchDifferenceSemver) + anyPatchAcceptedConstraint, _ := semver.NewConstraint(anyPatchAccepted) + lessThanNextMajorConstraint, _ := semver.NewConstraint(lessThanNextMajor) + + testCases := []struct { + name string + errExpected bool + mixins []manifest.MixinDeclaration + }{ + {"exact-semver", false, []manifest.MixinDeclaration{ + { + Name: mixin.ExampleMixinName, + Version: exampleMixinVersionConstraint, + }, + }}, + {"different-patch", true, []manifest.MixinDeclaration{ + { + Name: mixin.ExampleMixinName, + Version: patchDifferenceSemverConstraint, + }, + }}, + {"accept-different-patch", false, []manifest.MixinDeclaration{ + { + Name: mixin.ExampleMixinName, + Version: anyPatchAcceptedConstraint, + }, + }}, + {"accept-less-than-versions", false, []manifest.MixinDeclaration{ + { + Name: mixin.ExampleMixinName, + Version: lessThanNextMajorConstraint, + }, + }}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + m := &manifest.Manifest{ + Mixins: testCase.mixins, + } + results, err := l.Lint(context.Background(), m, testConfig) + if testCase.errExpected { + require.Error(t, err, "Linting should return an error") + tests.RequireOutputContains(t, err.Error(), fmt.Sprintf( + "mixin %s is installed at version v%s but your bundle requires version %s", + mixin.ExampleMixinName, + exampleMixinVersion, + testCase.mixins[0].Version.String(), + )) + } else { + require.NoError(t, err, "Linting should not return an error") + } + require.Len(t, results, 0, "linter should have returned 0 result") + }) + } + +} diff --git a/pkg/mixin/helpers.go b/pkg/mixin/helpers.go index 1a7283c09..5bda3ed7b 100644 --- a/pkg/mixin/helpers.go +++ b/pkg/mixin/helpers.go @@ -10,6 +10,7 @@ import ( "get.porter.sh/porter/pkg/pkgmgmt" "get.porter.sh/porter/pkg/pkgmgmt/client" "get.porter.sh/porter/pkg/portercontext" + "github.com/Masterminds/semver/v3" ) type TestMixinProvider struct { @@ -24,6 +25,10 @@ type TestMixinProvider struct { ReturnBuildError bool } +// hoist these into variables so tests can reference them safely +var ExampleMixinName = "testmixin" +var ExampleMixinSemver = semver.New(0, 1, 0, "", "") + // NewTestMixinProvider helps us test Porter.Mixins in our unit tests without actually hitting any real plugins on the file system. func NewTestMixinProvider() *TestMixinProvider { packages := []pkgmgmt.PackageMetadata{ @@ -36,9 +41,9 @@ func NewTestMixinProvider() *TestMixinProvider { }, }, &Metadata{ - Name: "testmixin", + Name: ExampleMixinName, VersionInfo: pkgmgmt.VersionInfo{ - Version: "v0.1.0", + Version: fmt.Sprintf("v%s", ExampleMixinSemver.String()), Commit: "abc123", Author: "Porter Authors", }, @@ -78,7 +83,7 @@ func (p *TestMixinProvider) GetSchema(ctx context.Context, name string) (string, switch name { case "exec": schemaFile = "../exec/schema/exec.json" - case "testmixin": + case ExampleMixinName: schemaFile = "../../cmd/testmixin/schema.json" default: return "", nil From 0cf7e48d419f9e45b7301b6edde75eb44e9b25c4 Mon Sep 17 00:00:00 2001 From: David Gannon <19214156+dgannon991@users.noreply.github.com> Date: Thu, 19 Sep 2024 09:55:47 +0100 Subject: [PATCH 3/3] Added a test with no version number Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com> --- pkg/linter/linter_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/linter/linter_test.go b/pkg/linter/linter_test.go index e0b27e0b1..1a08ec82d 100644 --- a/pkg/linter/linter_test.go +++ b/pkg/linter/linter_test.go @@ -439,6 +439,11 @@ func TestLinter_Lint_MixinVersions(t *testing.T) { Version: lessThanNextMajorConstraint, }, }}, + {"no-version-provided", false, []manifest.MixinDeclaration{ + { + Name: mixin.ExampleMixinName, + }, + }}, } for _, testCase := range testCases {