Skip to content

don't parse Jsonnet as YAML if it has runtime errors - #192

Open
vinayvinay wants to merge 1 commit into
incident-io:masterfrom
vinayvinay:v-dont-parse-jsonnet-with-runtime-errors
Open

don't parse Jsonnet as YAML if it has runtime errors#192
vinayvinay wants to merge 1 commit into
incident-io:masterfrom
vinayvinay:v-dont-parse-jsonnet-with-runtime-errors

Conversation

@vinayvinay

Copy link
Copy Markdown

fixes issue #66 reported by @lawrencejones.

(refreshing my memory on Go by doing something useful)

fixes issue reported by @lawrencejones:
incident-io#66

(refreshing my memory on Go by doing something useful)

@samstarling samstarling left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution — this makes sense, but there's an issue with the error handling, as I think we swallow all errors. Currently we use any error as a sign to just carry on and try the rest of the Parse function.

I wonder if we maybe want to introspect the suffix of the filename to try and work out which parser we should use? This would be a breaking change, however.

Comment thread source/parse.go
Comment on lines +24 to 30
if jsonString, err := jsonnet.MakeVM().EvaluateSnippet(filename, string(data)); err != nil {
if strings.HasPrefix(err.Error(), "RUNTIME ERROR") {
return []Entry{}, err
}
} else {
data = []byte(jsonString)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be a little neater (and more idiomatic) to do something like:

var jsonnetError jsonnet.RuntimeError
if errors.As(err, &jsonnetError) {
	// handle error
}

This code also swallows any other errors, and so I think we need to work out an approach for those cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants