From 4fc91dad7fcbebfb526da4919a39a27bca6faad4 Mon Sep 17 00:00:00 2001 From: Christopher Hunter Date: Sun, 12 Dec 2021 16:59:48 -0800 Subject: [PATCH] refactor (validate): move validations to cargo package --- .../validate/floating-release/Kilnfile | 5 - .../validate/floating-release/Kilnfile.lock | 9 -- .../validate/invalid-constraint/Kilnfile | 6 - .../validate/invalid-constraint/Kilnfile.lock | 9 -- .../testdata/validate/missing-lock/Kilnfile | 5 - .../testdata/validate/pinned-release/Kilnfile | 6 - .../validate/pinned-release/Kilnfile.lock | 9 -- .../validate/wrong-version-type/Kilnfile | 6 - .../validate/wrong-version-type/Kilnfile.lock | 9 -- internal/commands/validate.go | 51 +------ internal/commands/validate_test.go | 137 ----------------- pkg/cargo/kilnfile_validate.go | 59 +++++++ pkg/cargo/kilnfile_validate_test.go | 144 ++++++++++++++++++ 13 files changed, 207 insertions(+), 248 deletions(-) delete mode 100644 internal/commands/testdata/validate/floating-release/Kilnfile delete mode 100644 internal/commands/testdata/validate/floating-release/Kilnfile.lock delete mode 100644 internal/commands/testdata/validate/invalid-constraint/Kilnfile delete mode 100644 internal/commands/testdata/validate/invalid-constraint/Kilnfile.lock delete mode 100644 internal/commands/testdata/validate/missing-lock/Kilnfile delete mode 100644 internal/commands/testdata/validate/pinned-release/Kilnfile delete mode 100644 internal/commands/testdata/validate/pinned-release/Kilnfile.lock delete mode 100644 internal/commands/testdata/validate/wrong-version-type/Kilnfile delete mode 100644 internal/commands/testdata/validate/wrong-version-type/Kilnfile.lock delete mode 100644 internal/commands/validate_test.go create mode 100644 pkg/cargo/kilnfile_validate.go create mode 100644 pkg/cargo/kilnfile_validate_test.go diff --git a/internal/commands/testdata/validate/floating-release/Kilnfile b/internal/commands/testdata/validate/floating-release/Kilnfile deleted file mode 100644 index 1b506e594..000000000 --- a/internal/commands/testdata/validate/floating-release/Kilnfile +++ /dev/null @@ -1,5 +0,0 @@ -slug: example -release_sources: - - type: bosh.io -releases: - - name: bpm diff --git a/internal/commands/testdata/validate/floating-release/Kilnfile.lock b/internal/commands/testdata/validate/floating-release/Kilnfile.lock deleted file mode 100644 index 8f39bb76b..000000000 --- a/internal/commands/testdata/validate/floating-release/Kilnfile.lock +++ /dev/null @@ -1,9 +0,0 @@ -releases: - - name: bpm - sha1: 502e9446fa34accaf122ad2b28b6ffa543d5bbca - version: 1.1.12 - remote_source: bosh.io - remote_path: https://bosh.io/d/github.com/cloudfoundry/bpm-release?v=1.1.12 -stemcell_criteria: - os: ubuntu-xenial - version: "621.129" diff --git a/internal/commands/testdata/validate/invalid-constraint/Kilnfile b/internal/commands/testdata/validate/invalid-constraint/Kilnfile deleted file mode 100644 index d04261117..000000000 --- a/internal/commands/testdata/validate/invalid-constraint/Kilnfile +++ /dev/null @@ -1,6 +0,0 @@ -slug: example -release_sources: - - type: bosh.io -releases: - - name: bpm - version: not-a-constraint diff --git a/internal/commands/testdata/validate/invalid-constraint/Kilnfile.lock b/internal/commands/testdata/validate/invalid-constraint/Kilnfile.lock deleted file mode 100644 index 8f39bb76b..000000000 --- a/internal/commands/testdata/validate/invalid-constraint/Kilnfile.lock +++ /dev/null @@ -1,9 +0,0 @@ -releases: - - name: bpm - sha1: 502e9446fa34accaf122ad2b28b6ffa543d5bbca - version: 1.1.12 - remote_source: bosh.io - remote_path: https://bosh.io/d/github.com/cloudfoundry/bpm-release?v=1.1.12 -stemcell_criteria: - os: ubuntu-xenial - version: "621.129" diff --git a/internal/commands/testdata/validate/missing-lock/Kilnfile b/internal/commands/testdata/validate/missing-lock/Kilnfile deleted file mode 100644 index 1b506e594..000000000 --- a/internal/commands/testdata/validate/missing-lock/Kilnfile +++ /dev/null @@ -1,5 +0,0 @@ -slug: example -release_sources: - - type: bosh.io -releases: - - name: bpm diff --git a/internal/commands/testdata/validate/pinned-release/Kilnfile b/internal/commands/testdata/validate/pinned-release/Kilnfile deleted file mode 100644 index 63e3a458f..000000000 --- a/internal/commands/testdata/validate/pinned-release/Kilnfile +++ /dev/null @@ -1,6 +0,0 @@ -slug: example -release_sources: - - type: bosh.io -releases: - - name: bpm - version: ~1.1 diff --git a/internal/commands/testdata/validate/pinned-release/Kilnfile.lock b/internal/commands/testdata/validate/pinned-release/Kilnfile.lock deleted file mode 100644 index 8f39bb76b..000000000 --- a/internal/commands/testdata/validate/pinned-release/Kilnfile.lock +++ /dev/null @@ -1,9 +0,0 @@ -releases: - - name: bpm - sha1: 502e9446fa34accaf122ad2b28b6ffa543d5bbca - version: 1.1.12 - remote_source: bosh.io - remote_path: https://bosh.io/d/github.com/cloudfoundry/bpm-release?v=1.1.12 -stemcell_criteria: - os: ubuntu-xenial - version: "621.129" diff --git a/internal/commands/testdata/validate/wrong-version-type/Kilnfile b/internal/commands/testdata/validate/wrong-version-type/Kilnfile deleted file mode 100644 index e8bf1dc04..000000000 --- a/internal/commands/testdata/validate/wrong-version-type/Kilnfile +++ /dev/null @@ -1,6 +0,0 @@ -slug: example -release_sources: - - type: bosh.io -releases: - - name: bpm - version: {} # strings expected diff --git a/internal/commands/testdata/validate/wrong-version-type/Kilnfile.lock b/internal/commands/testdata/validate/wrong-version-type/Kilnfile.lock deleted file mode 100644 index 8f39bb76b..000000000 --- a/internal/commands/testdata/validate/wrong-version-type/Kilnfile.lock +++ /dev/null @@ -1,9 +0,0 @@ -releases: - - name: bpm - sha1: 502e9446fa34accaf122ad2b28b6ffa543d5bbca - version: 1.1.12 - remote_source: bosh.io - remote_path: https://bosh.io/d/github.com/cloudfoundry/bpm-release?v=1.1.12 -stemcell_criteria: - os: ubuntu-xenial - version: "621.129" diff --git a/internal/commands/validate.go b/internal/commands/validate.go index 1a2b6254b..f38f3114b 100644 --- a/internal/commands/validate.go +++ b/internal/commands/validate.go @@ -4,7 +4,6 @@ import ( "fmt" "strings" - "github.com/Masterminds/semver" "github.com/go-git/go-billy/v5" "github.com/pivotal-cf/jhanda" @@ -34,28 +33,14 @@ func (v Validate) Execute(args []string) error { return err } - kilnfile, lock, err := v.Options.Standard.LoadKilnfiles(v.FS, nil) + kf, lock, err := v.Options.Standard.LoadKilnfiles(v.FS, nil) if err != nil { return fmt.Errorf("failed to load kilnfiles: %w", err) } - var releaseErrors errorList - - for index, release := range kilnfile.Releases { - releaseLock, err := lock.FindReleaseWithName(release.Name) - if err != nil { - releaseErrors = append(releaseErrors, - fmt.Errorf("release %q not found in lock", release.Name)) - continue - } - - if err := validateRelease(release, releaseLock, index); err != nil { - releaseErrors = append(releaseErrors, err) - } - } - - if len(releaseErrors) > 0 { - return releaseErrors + errs := cargo.Validate(kf, lock) + if len(errs) > 0 { + return errorList(errs) } return nil @@ -71,34 +56,6 @@ func (list errorList) Error() string { return strings.Join(messages, "\n") } -func validateRelease(release cargo.ComponentSpec, lock cargo.ComponentLock, index int) error { - if release.Name == "" { - return fmt.Errorf("release at index %d missing name", index) - } - - if release.Version != "" { - c, err := semver.NewConstraint(release.Version) - if err != nil { - return fmt.Errorf("release %s (index %d in Kilnfile) has invalid version constraint: %w", - release.Name, index, err) - } - - v, err := semver.NewVersion(lock.Version) - if err != nil { - return fmt.Errorf("release %s (index %d in Kilnfile.lock) has invalid lock version %q: %w", - release.Name, index, lock.Version, err) - } - - matches, errs := c.Validate(v) - if !matches { - return fmt.Errorf("release %s version in lock %q does not match constraint %q: %v", - release.Name, lock.Version, release.Version, errs) - } - } - - return nil -} - func (v Validate) Usage() jhanda.Usage { return jhanda.Usage{ Description: "Validate checks for common Kilnfile and Kilnfile.lock mistakes", diff --git a/internal/commands/validate_test.go b/internal/commands/validate_test.go deleted file mode 100644 index 6fd1f7a9d..000000000 --- a/internal/commands/validate_test.go +++ /dev/null @@ -1,137 +0,0 @@ -package commands - -import ( - "testing" - - "github.com/pivotal-cf/kiln/pkg/cargo" - - "github.com/go-git/go-billy/v5/osfs" - Ω "github.com/onsi/gomega" -) - -func TestValidate_FloatingRelease(t *testing.T) { - t.Parallel() - please := Ω.NewWithT(t) - validate := Validate{ - FS: osfs.New("testdata/validate/floating-release"), - } - err := validate.Execute(nil) - please.Expect(err).NotTo(Ω.HaveOccurred()) -} - -func TestValidate_MissingLock(t *testing.T) { - t.Parallel() - please := Ω.NewWithT(t) - validate := Validate{ - FS: osfs.New("testdata/validate/missing-lock"), - } - err := validate.Execute(nil) - please.Expect(err).To(Ω.HaveOccurred()) -} - -func TestValidate_WrongVersionType(t *testing.T) { - t.Parallel() - please := Ω.NewWithT(t) - validate := Validate{ - FS: osfs.New("testdata/validate/wrong-version-type"), - } - err := validate.Execute(nil) - please.Expect(err).To(Ω.HaveOccurred()) -} - -func TestValidate_InvalidConstraint(t *testing.T) { - t.Parallel() - please := Ω.NewWithT(t) - validate := Validate{ - FS: osfs.New("testdata/validate/invalid-constraint"), - } - err := validate.Execute(nil) - please.Expect(err).To(Ω.MatchError(Ω.ContainSubstring("bpm"))) -} - -func TestValidate_PinnedRelease(t *testing.T) { - t.Parallel() - please := Ω.NewWithT(t) - validate := Validate{ - FS: osfs.New("testdata/validate/pinned-release"), - } - err := validate.Execute(nil) - please.Expect(err).NotTo(Ω.HaveOccurred()) -} - -func TestValidate_validateRelease(t *testing.T) { - t.Run("missing name", func(t *testing.T) { - please := Ω.NewWithT(t) - r := cargo.ComponentSpec{} - l := cargo.ComponentLock{} - err := validateRelease(r, l, 0) - please.Expect(err).To(Ω.And( - Ω.HaveOccurred(), - Ω.MatchError(Ω.ContainSubstring("missing name")), - )) - }) - - t.Run("no version", func(t *testing.T) { - please := Ω.NewWithT(t) - r := cargo.ComponentSpec{ - Name: "capi", - } - l := cargo.ComponentLock{ - Name: "capi", - Version: "2.3.4", - } - err := validateRelease(r, l, 0) - please.Expect(err).NotTo(Ω.HaveOccurred()) - }) - - t.Run("invalid version constraint", func(t *testing.T) { - please := Ω.NewWithT(t) - r := cargo.ComponentSpec{ - Name: "capi", - Version: "meh", - } - l := cargo.ComponentLock{ - Name: "capi", - Version: "2.3.4", - } - err := validateRelease(r, l, 0) - please.Expect(err).To(Ω.And( - Ω.HaveOccurred(), - Ω.MatchError(Ω.ContainSubstring("invalid version constraint")), - )) - }) - - t.Run("version does not match constraint", func(t *testing.T) { - please := Ω.NewWithT(t) - r := cargo.ComponentSpec{ - Name: "capi", - Version: "~2", - } - l := cargo.ComponentLock{ - Name: "capi", - Version: "3.0.5", - } - err := validateRelease(r, l, 0) - please.Expect(err).To(Ω.And( - Ω.HaveOccurred(), - Ω.MatchError(Ω.ContainSubstring("match constraint")), - )) - }) - - t.Run("invalid lock version", func(t *testing.T) { - please := Ω.NewWithT(t) - r := cargo.ComponentSpec{ - Name: "capi", - Version: "~2", - } - l := cargo.ComponentLock{ - Name: "capi", - Version: "BAD", - } - err := validateRelease(r, l, 0) - please.Expect(err).To(Ω.And( - Ω.HaveOccurred(), - Ω.MatchError(Ω.ContainSubstring("invalid lock version")), - )) - }) -} diff --git a/pkg/cargo/kilnfile_validate.go b/pkg/cargo/kilnfile_validate.go new file mode 100644 index 000000000..552ab9c5a --- /dev/null +++ b/pkg/cargo/kilnfile_validate.go @@ -0,0 +1,59 @@ +package cargo + +import ( + "fmt" + + "github.com/Masterminds/semver" +) + +func Validate(spec Kilnfile, lock KilnfileLock) []error { + var result []error + + for index, componentSpec := range spec.Releases { + if componentSpec.Name == "" { + result = append(result, fmt.Errorf("spec at index %d missing name", index)) + continue + } + + componentLock, err := lock.FindReleaseWithName(componentSpec.Name) + if err != nil { + result = append(result, + fmt.Errorf("component spec for release %q not found in lock", componentSpec.Name)) + continue + } + + if err := checkComponentVersionsAndConstraint(componentSpec, componentLock, index); err != nil { + result = append(result, err) + } + } + + if len(result) > 0 { + return result + } + + return nil +} + +func checkComponentVersionsAndConstraint(spec ComponentSpec, lock ComponentLock, index int) error { + v, err := semver.NewVersion(lock.Version) + if err != nil { + return fmt.Errorf("spec %s (index %d in Kilnfile.lock) has invalid lock version %q: %w", + spec.Name, index, lock.Version, err) + } + + if spec.Version != "" { + c, err := semver.NewConstraint(spec.Version) + if err != nil { + return fmt.Errorf("spec %s (index %d in Kilnfile) has invalid version constraint: %w", + spec.Name, index, err) + } + + matches, errs := c.Validate(v) + if !matches { + return fmt.Errorf("spec %s version in lock %q does not match constraint %q: %v", + spec.Name, lock.Version, spec.Version, errs) + } + } + + return nil +} diff --git a/pkg/cargo/kilnfile_validate_test.go b/pkg/cargo/kilnfile_validate_test.go new file mode 100644 index 000000000..648532738 --- /dev/null +++ b/pkg/cargo/kilnfile_validate_test.go @@ -0,0 +1,144 @@ +package cargo + +import ( + "testing" + + Ω "github.com/onsi/gomega" +) + +func TestValidate_MissingName(t *testing.T) { + t.Parallel() + please := Ω.NewWithT(t) + results := Validate(Kilnfile{ + Releases: []ComponentSpec{ + {}, + }, + }, KilnfileLock{ + Releases: []ComponentLock{ + {Name: "banana", Version: "1.2.3"}, + }, + }) + please.Expect(results).To(Ω.HaveLen(1)) +} + +func TestValidate_FloatingRelease(t *testing.T) { + t.Parallel() + please := Ω.NewWithT(t) + results := Validate(Kilnfile{ + Releases: []ComponentSpec{ + {Name: "banana", Version: "1.1.*"}, + }, + }, KilnfileLock{ + Releases: []ComponentLock{ + {Name: "banana", Version: "1.1.12"}, + }, + }) + please.Expect(results).To(Ω.HaveLen(0)) +} + +func TestValidate_MissingLock(t *testing.T) { + t.Parallel() + please := Ω.NewWithT(t) + results := Validate(Kilnfile{ + Releases: []ComponentSpec{ + {Name: "banana", Version: "1.1.*"}, + }, + }, KilnfileLock{}) + please.Expect(results).To(Ω.HaveLen(1)) +} + +func TestValidate_InvalidConstraint(t *testing.T) { + t.Parallel() + please := Ω.NewWithT(t) + results := Validate(Kilnfile{ + Releases: []ComponentSpec{ + {Name: "banana", Version: "NOT A CONSTRAINT"}, + }, + }, KilnfileLock{ + Releases: []ComponentLock{ + {Name: "banana", Version: "1.2.3"}, + }, + }) + please.Expect(results).To(Ω.HaveLen(1)) +} + +func TestValidate_PinnedRelease(t *testing.T) { + t.Parallel() + please := Ω.NewWithT(t) + results := Validate(Kilnfile{ + Releases: []ComponentSpec{ + {Name: "banana", Version: "1.2.3"}, + }, + }, KilnfileLock{ + Releases: []ComponentLock{ + {Name: "banana", Version: "1.2.3"}, + }, + }) + please.Expect(results).To(Ω.HaveLen(0)) +} + +func TestValidate_checkComponentVersionsAndConstraint(t *testing.T) { + t.Run("no version", func(t *testing.T) { + please := Ω.NewWithT(t) + r := ComponentSpec{ + Name: "capi", + } + l := ComponentLock{ + Name: "capi", + Version: "2.3.4", + } + err := checkComponentVersionsAndConstraint(r, l, 0) + please.Expect(err).NotTo(Ω.HaveOccurred()) + }) + + t.Run("invalid version constraint", func(t *testing.T) { + please := Ω.NewWithT(t) + r := ComponentSpec{ + Name: "capi", + Version: "meh", + } + l := ComponentLock{ + Name: "capi", + Version: "2.3.4", + } + err := checkComponentVersionsAndConstraint(r, l, 0) + please.Expect(err).To(Ω.And( + Ω.HaveOccurred(), + Ω.MatchError(Ω.ContainSubstring("invalid version constraint")), + )) + }) + + t.Run("version does not match constraint", func(t *testing.T) { + please := Ω.NewWithT(t) + r := ComponentSpec{ + Name: "capi", + Version: "~2", + } + l := ComponentLock{ + Name: "capi", + Version: "3.0.5", + } + err := checkComponentVersionsAndConstraint(r, l, 0) + please.Expect(err).To(Ω.And( + Ω.HaveOccurred(), + Ω.MatchError(Ω.ContainSubstring("match constraint")), + )) + }) + + t.Run("invalid lock version", func(t *testing.T) { + please := Ω.NewWithT(t) + r := ComponentSpec{ + Name: "capi", + Version: "~2", + } + l := ComponentLock{ + Name: "capi", + Version: "BAD", + } + err := checkComponentVersionsAndConstraint(r, l, 0) + please.Expect(err).To(Ω.And( + Ω.HaveOccurred(), + Ω.MatchError(Ω.ContainSubstring("invalid lock version")), + )) + }) +}