Skip to content

campaigns: move validation code to campaignutils #353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 20, 2020
Merged

campaigns: move validation code to campaignutils #353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions cmd/src/campaigns_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,12 +193,8 @@ func campaignsExecute(ctx context.Context, out *output.Output, svc *campaigns.Se
}

pending := campaignsCreatePending(out, "Parsing campaign spec")
campaignSpec, rawSpec, err := svc.ParseCampaignSpec(specFile)
campaignSpec, rawSpec, err := campaignsParseSpec(out, svc, specFile)
if err != nil {
return "", "", errors.Wrap(err, "parsing campaign spec")
}

if err := campaignsValidateSpec(out, campaignSpec); err != nil {
return "", "", err
}
campaignsCompletePending(pending, "Parsing campaign spec")
Expand Down Expand Up @@ -290,6 +286,34 @@ func campaignsExecute(ctx context.Context, out *output.Output, svc *campaigns.Se
return id, url, nil
}

// campaignsParseSpec parses and validates the given campaign spec. If the spec
// has validation errors, the errors are output in a human readable form and an
// exitCodeError is returned.
func campaignsParseSpec(out *output.Output, svc *campaigns.Service, input io.ReadCloser) (*campaigns.CampaignSpec, string, error) {
spec, raw, err := svc.ParseCampaignSpec(input)
if err != nil {
if merr, ok := err.(*multierror.Error); ok {
block := out.Block(output.Line("\u274c", output.StyleWarning, "Campaign spec failed validation."))
defer block.Close()

for i, err := range merr.Errors {
block.Writef("%d. %s", i+1, err)
}

return nil, "", &exitCodeError{
error: nil,
exitCode: 2,
}
} else {
// This shouldn't happen; let's just punt and let the normal
// rendering occur.
return nil, "", err
}
}

return spec, raw, nil
}
Comment on lines +292 to +315
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if we should incorporate this logic into ParseCampaignSpec, then we wouldn't need this function that gets passed in a service, and it can't be forgotten. Are there use-cases where we explicitly would not want to have this error prettify logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's basically a layering thing: I don't want internal/campaigns (in this case, the Service methods) to have any specific knowledge of internal/output, so this code lives here to basically handle the rendering of any errors. This function is used by each of the campaign commands that have to parse a spec file, though, so it's still shared.


// printExecutionError is used to print the possible error returned by
// campaignsExecute.
func printExecutionError(out *output.Output, err error) {
Expand Down
8 changes: 2 additions & 6 deletions cmd/src/campaigns_repositories.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,9 @@ Examples:
return err
}

spec, _, err := svc.ParseCampaignSpec(specFile)
if err != nil {
return errors.Wrap(err, "parsing campaign spec")
}

out := output.NewOutput(flagSet.Output(), output.OutputOpts{Verbose: *verbose})
if err := campaignsValidateSpec(out, spec); err != nil {
spec, _, err := campaignsParseSpec(out, svc, specFile)
if err != nil {
return err
}

Expand Down
36 changes: 1 addition & 35 deletions cmd/src/campaigns_validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import (
"flag"
"fmt"

"github.com/hashicorp/go-multierror"
"github.com/pkg/errors"
"github.com/sourcegraph/src-cli/internal/campaigns"
"github.com/sourcegraph/src-cli/internal/output"
)
Expand Down Expand Up @@ -40,13 +38,8 @@ Examples:

svc := campaigns.NewService(&campaigns.ServiceOpts{})

spec, _, err := svc.ParseCampaignSpec(specFile)
if err != nil {
return errors.Wrap(err, "parsing campaign spec")
}

out := output.NewOutput(flagSet.Output(), output.OutputOpts{Verbose: *verbose})
if err := campaignsValidateSpec(out, spec); err != nil {
if _, _, err := campaignsParseSpec(out, svc, specFile); err != nil {
return err
}

Expand All @@ -64,30 +57,3 @@ Examples:
},
})
}

// campaignsValidateSpec validates the given campaign spec. If the spec has
// validation errors, they are output in a human readable form and an
// exitCodeError is returned.
func campaignsValidateSpec(out *output.Output, spec *campaigns.CampaignSpec) error {
if err := spec.Validate(); err != nil {
if merr, ok := err.(*multierror.Error); ok {
block := out.Block(output.Line("\u274c", output.StyleWarning, "Campaign spec failed validation."))
defer block.Close()

for i, err := range merr.Errors {
block.Writef("%d. %s", i+1, err)
}

return &exitCodeError{
error: nil,
exitCode: 2,
}
} else {
// This shouldn't happen; let's just punt and let the normal
// rendering occur.
return err
}
}

return nil
}
6 changes: 4 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,19 @@ require (
github.com/olekukonko/tablewriter v0.0.4 // indirect
github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4
github.com/pkg/errors v0.9.1
github.com/sourcegraph/campaignutils v0.0.0-20201014140011-721315691938
github.com/sourcegraph/campaignutils v0.0.0-20201016010611-63eb2bca27ad
github.com/sourcegraph/codeintelutils v0.0.0-20200824140252-1db3aed5cf58
github.com/sourcegraph/go-diff v0.6.0
github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf
github.com/ssor/bom v0.0.0-20170718123548-6386211fdfcf // indirect
github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
github.com/xeipuuv/gojsonschema v1.2.0
golang.org/x/net v0.0.0-20200625001655-4c5254603344
golang.org/x/sys v0.0.0-20200622214017-ed371f2e16b4
gopkg.in/yaml.v2 v2.3.0
jaytaylor.com/html2text v0.0.0-20200412013138-3577fbdbcff7
)

replace github.com/gosuri/uilive v0.0.4 => github.com/mrnugget/uilive v0.0.4-fix-escape

// See: https://github.com/ghodss/yaml/pull/65
replace github.com/ghodss/yaml => github.com/sourcegraph/yaml v1.0.1-0.20200714132230-56936252f152
9 changes: 7 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,16 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/shurcooL/go v0.0.0-20180423040247-9e1955d9fb6e/go.mod h1:TDJrrUr11Vxrven61rcy3hJMUqaf/CLWYhHNPmT14Lk=
github.com/shurcooL/go-goon v0.0.0-20170922171312-37c2f522c041/go.mod h1:N5mDOmsrJOB+vfqUK+7DmDyjhSLIIBnXo9lvZJj3MWQ=
github.com/sourcegraph/campaignutils v0.0.0-20201014140011-721315691938 h1:h5gOw7sMlYFSQFNdFPXNHJ4dtD0nlgClmL8SGAzlNM4=
github.com/sourcegraph/campaignutils v0.0.0-20201014140011-721315691938/go.mod h1:5P8k8KlKz78RZJ2EFk9k9ln/j/twj28z/+BvO5hHZJ8=
github.com/sourcegraph/campaignutils v0.0.0-20201016010611-63eb2bca27ad h1:HeSWFpxau4Jqk0s4yEhOdep+KYrJDm0497uhb/hnsgU=
github.com/sourcegraph/campaignutils v0.0.0-20201016010611-63eb2bca27ad/go.mod h1:xm6i78Mk2t4DBLQDqEFc/3x6IPf7yYZCgbNaTQGhJHA=
github.com/sourcegraph/codeintelutils v0.0.0-20200824140252-1db3aed5cf58 h1:Ps+U1xoZP+Zoph39YfRB6Q846ghO8pgrAgp0MiObvPs=
github.com/sourcegraph/codeintelutils v0.0.0-20200824140252-1db3aed5cf58/go.mod h1:HplI8gRslTrTUUsSYwu28hSOderix7m5dHNca7xBzeo=
github.com/sourcegraph/go-diff v0.6.0 h1:WbN9e/jD8ujU+o0vd9IFN5AEwtfB0rn/zM/AANaClqQ=
github.com/sourcegraph/go-diff v0.6.0/go.mod h1:iBszgVvyxdc8SFZ7gm69go2KDdt3ag071iBaWPF6cjs=
github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf h1:oAdWFqhStsWiiMP/vkkHiMXqFXzl1XfUNOdxKJbd6bI=
github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf/go.mod h1:ppFaPm6kpcHnZGqQTFhUIAQRIEhdQDWP1PCv4/ON354=
github.com/sourcegraph/yaml v1.0.1-0.20200714132230-56936252f152 h1:z/MpntplPaW6QW95pzcAR/72Z5TWDyDnSo0EOcyij9o=
github.com/sourcegraph/yaml v1.0.1-0.20200714132230-56936252f152/go.mod h1:GIjDIg/heH5DOkXY3YJ/wNhfHsQHoXGjl8G8amsYQ1I=
github.com/ssor/bom v0.0.0-20170718123548-6386211fdfcf h1:pvbZ0lM0XWPBqUKqFU8cmavspvIl9nulOYwdy6IFRRo=
github.com/ssor/bom v0.0.0-20170718123548-6386211fdfcf/go.mod h1:RJID2RhlZKId02nZ62WenDCkgHFerpIOmW0iT7GKmXM=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
Expand Down Expand Up @@ -102,7 +104,10 @@ golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IV
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
gopkg.in/yaml.v2 v2.3.0 h1:clyUAQHOM3G0M3f5vQj7LuJrETvjVot3Z5el9nffUtU=
gopkg.in/yaml.v2 v2.3.0/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776 h1:tQIYjPdBoyREyB9XMu+nnTclpTYkz2zFM+lzLJFO4gQ=
gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
jaytaylor.com/html2text v0.0.0-20200412013138-3577fbdbcff7 h1:mub0MmFLOn8XLikZOAhgLD1kXJq8jgftSrrv7m00xFo=
jaytaylor.com/html2text v0.0.0-20200412013138-3577fbdbcff7/go.mod h1:OxvTsCwKosqQ1q7B+8FwXqg4rKZ/UG9dUW+g/VL2xH4=
35 changes: 2 additions & 33 deletions internal/campaigns/campaign_spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,9 @@ package campaigns
import (
"fmt"

"github.com/hashicorp/go-multierror"
"github.com/pkg/errors"
"github.com/sourcegraph/campaignutils/overridable"
"github.com/sourcegraph/campaignutils/yaml"
"github.com/sourcegraph/src-cli/schema"
"github.com/xeipuuv/gojsonschema"
"gopkg.in/yaml.v2"
)

// Some general notes about the struct definitions below.
Expand Down Expand Up @@ -75,41 +72,13 @@ type Step struct {

func ParseCampaignSpec(data []byte) (*CampaignSpec, error) {
var spec CampaignSpec
if err := yaml.Unmarshal(data, &spec); err != nil {
if err := yaml.UnmarshalValidate(schema.CampaignSpecJSON, data, &spec); err != nil {
return nil, err
}

return &spec, nil
}

var campaignSpecSchema *gojsonschema.Schema

func (spec *CampaignSpec) Validate() error {
if campaignSpecSchema == nil {
var err error
campaignSpecSchema, err = gojsonschema.NewSchemaLoader().Compile(gojsonschema.NewStringLoader(schema.CampaignSpecJSON))
if err != nil {
return errors.Wrap(err, "parsing campaign spec schema")
}
}

result, err := campaignSpecSchema.Validate(gojsonschema.NewGoLoader(spec))
if err != nil {
return errors.Wrapf(err, "validating campaign spec")
}
if result.Valid() {
return nil
}

var errs *multierror.Error
for _, verr := range result.Errors() {
// ResultError instances don't actually implement error, so we need to
// wrap them as best we can before adding them to the multierror.
errs = multierror.Append(errs, errors.New(verr.String()))
}
return errs
}

func (on *OnQueryOrRepository) String() string {
if on.RepositoriesMatchingQuery != "" {
return on.RepositoriesMatchingQuery
Expand Down