Skip to content
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

add formatting for hcl parsing error messages #5972

Merged
merged 4 commits into from
Jul 19, 2019
Merged

add formatting for hcl parsing error messages #5972

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
22732ab
add formatting for hcl parsing error messages
jazzyfresh Jul 17, 2019
c0bb2af
ParseHclInterface test for invalid labels
jazzyfresh Jul 17, 2019
b473b3b
fix task event errors
jazzyfresh Jul 17, 2019
90d72e0
fix broken test by adding TaskDriverFailure and refactor for code review
jazzyfresh Jul 19, 2019
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
9 changes: 6 additions & 3 deletions client/allocrunner/taskrunner/task_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,6 @@ MAIN:
// Run the task
if err := tr.runDriver(); err != nil {
tr.logger.Error("running driver failed", "error", err)
tr.EmitEvent(structs.NewTaskEvent(structs.TaskDriverFailure).SetDriverError(err))
nickethier marked this conversation as resolved.
Show resolved Hide resolved
tr.restartTracker.SetStartError(err)
goto RESTART
}
Expand Down Expand Up @@ -705,12 +704,14 @@ func (tr *TaskRunner) runDriver() error {
tr.logger.Warn("some environment variables not available for rendering", "keys", strings.Join(keys, ", "))
}

val, diag := hclutils.ParseHclInterface(tr.task.Config, tr.taskSchema, vars)
val, diag, diagErrs := hclutils.ParseHclInterface(tr.task.Config, tr.taskSchema, vars)
if diag.HasErrors() {
return multierror.Append(errors.New("failed to parse config"), diag.Errs()...)
tr.EmitEvent(structs.NewTaskEvent(structs.TaskFailedValidation).SetDriverError(err))
Copy link
Member

Choose a reason for hiding this comment

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

I can't tell where this err comes from, is it the correct error to use here or should it be something from diagErrs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you, good catch! I am supposed to pass in the multierror that gets created in the next line, but blindly copy-pastad and forgot to change it. This was part of an attempt to create more granular task events (eg. TaskFailedValidation for config parsing failures, and TaskDriverFailure for failures that were because of the driver), so I moved the event emission out of MAIN: and into runDriver(). I am not sure if that is too messy for a small change like this, so I can put the task events back the way they were...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is fixed, but please give a double check

return multierror.Append(errors.New("failed to parse config"), diagErrs...)
}

if err := taskConfig.EncodeDriverConfig(val); err != nil {
tr.EmitEvent(structs.NewTaskEvent(structs.TaskFailedValidation).SetDriverError(err))
return fmt.Errorf("failed to encode driver config: %v", err)
}

Expand All @@ -734,11 +735,13 @@ func (tr *TaskRunner) runDriver() error {
if err == bstructs.ErrPluginShutdown {
tr.logger.Info("failed to start task because plugin shutdown unexpectedly; attempting to recover")
if err := tr.initDriver(); err != nil {
tr.EmitEvent(structs.NewTaskEvent(structs.TaskDriverFailure).SetDriverError(err))
return fmt.Errorf("failed to initialize driver after it exited unexpectedly: %v", err)
}

handle, net, err = tr.driver.StartTask(taskConfig)
if err != nil {
tr.EmitEvent(structs.NewTaskEvent(structs.TaskDriverFailure).SetDriverError(err))
return fmt.Errorf("failed to start task after driver exited unexpectedly: %v", err)
}
} else {
Expand Down
2 changes: 1 addition & 1 deletion helper/pluginutils/hclutils/testing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (b *HCLParser) parse(t *testing.T, config, out interface{}) {
decSpec, diags := hclspecutils.Convert(b.spec)
require.Empty(t, diags)

ctyValue, diag := ParseHclInterface(config, decSpec, b.vars)
ctyValue, diag, _ := ParseHclInterface(config, decSpec, b.vars)
Copy link
Contributor

Choose a reason for hiding this comment

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

use err instead of _ here, and require.Nil on the error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

require.Empty(t, diag)

// encode
Expand Down
33 changes: 24 additions & 9 deletions helper/pluginutils/hclutils/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package hclutils

import (
"bytes"
"errors"
"fmt"

"github.com/hashicorp/hcl2/hcl"
Expand All @@ -17,7 +18,7 @@ import (
// ParseHclInterface is used to convert an interface value representing a hcl2
// body and return the interpolated value. Vars may be nil if there are no
// variables to interpolate.
func ParseHclInterface(val interface{}, spec hcldec.Spec, vars map[string]cty.Value) (cty.Value, hcl.Diagnostics) {
func ParseHclInterface(val interface{}, spec hcldec.Spec, vars map[string]cty.Value) (cty.Value, hcl.Diagnostics, []error) {
evalCtx := &hcl.EvalContext{
Variables: vars,
Functions: GetStdlibFuncs(),
Expand All @@ -29,27 +30,29 @@ func ParseHclInterface(val interface{}, spec hcldec.Spec, vars map[string]cty.Va
err := enc.Encode(val)
if err != nil {
// Convert to a hcl diagnostics message
return cty.NilVal, hcl.Diagnostics([]*hcl.Diagnostic{
{
errorMessage := fmt.Sprintf("Label encoding failed: %v", err)
return cty.NilVal,
hcl.Diagnostics([]*hcl.Diagnostic{{
Severity: hcl.DiagError,
Summary: "Failed to JSON encode value",
Detail: fmt.Sprintf("JSON encoding failed: %v", err),
}})
Summary: "Failed to encode label value",
Detail: errorMessage,
}}),
[]error{errors.New(errorMessage)}
}

// Parse the json as hcl2
hclFile, diag := hjson.Parse(buf.Bytes(), "")
if diag.HasErrors() {
return cty.NilVal, diag
return cty.NilVal, diag, formattedDiagnosticErrors(diag)
}

value, decDiag := hcldec.Decode(hclFile.Body, spec, evalCtx)
diag = diag.Extend(decDiag)
if diag.HasErrors() {
return cty.NilVal, diag
return cty.NilVal, diag, formattedDiagnosticErrors(diag)
}

return value, diag
return value, diag, nil
}

// GetStdlibFuncs returns the set of stdlib functions.
Expand All @@ -72,3 +75,15 @@ func GetStdlibFuncs() map[string]function.Function {
"upper": stdlib.UpperFunc,
}
}

func formattedDiagnosticErrors(diag hcl.Diagnostics) []error {
var errs []error
for _, d := range diag {
if d.Summary == "Extraneous JSON object property" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a comment pointing to where this text originates, so future us and notice when it changes or if this is no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Added the comment before the method

d.Summary = "Invalid label"
}
err := errors.New(fmt.Sprintf("%s: %s", d.Summary, d.Detail))
errs = append(errs, err)
}
return errs
}
6 changes: 3 additions & 3 deletions helper/pluginutils/hclutils/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,9 +358,9 @@ func TestParseHclInterface_Hcl(t *testing.T) {
t.Run(c.name, func(t *testing.T) {
t.Logf("Val: % #v", pretty.Formatter(c.config))
// Parse the interface
ctyValue, diag := hclutils.ParseHclInterface(c.config, c.spec, c.vars)
ctyValue, diag, errs := hclutils.ParseHclInterface(c.config, c.spec, c.vars)
if diag.HasErrors() {
for _, err := range diag.Errs() {
for _, err := range errs {
t.Error(err)
}
t.FailNow()
Expand Down Expand Up @@ -497,7 +497,7 @@ func TestParseUnknown(t *testing.T) {
t.Run(c.name, func(t *testing.T) {
inter := hclutils.HclConfigToInterface(t, c.hcl)

ctyValue, diag := hclutils.ParseHclInterface(inter, cSpec, vars)
ctyValue, diag, _ := hclutils.ParseHclInterface(inter, cSpec, vars)
t.Logf("parsed: %# v", pretty.Formatter(ctyValue))

require.True(t, diag.HasErrors())
Expand Down
7 changes: 4 additions & 3 deletions helper/pluginutils/loader/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,10 +457,11 @@ func (l *PluginLoader) validatePluginConfig(id PluginID, info *pluginInfo) error
}

// Parse the config using the spec
val, diag := hclutils.ParseHclInterface(info.config, spec, nil)
val, diag, diagErrs := hclutils.ParseHclInterface(info.config, spec, nil)
if diag.HasErrors() {
multierror.Append(&mErr, diag.Errs()...)
return multierror.Prefix(&mErr, "failed parsing config:")
multierror.Append(&mErr, diagErrs...)
return multierror.Prefix(&mErr, "failed to parse config")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd keep the trailing colon here, as Prefix() prefixes all elements rather than just once on top. I have a sample output in https://play.golang.org/p/sj0JcGRRVGH .

Though, I'd suggest avoiding multierror formatting. I find it awkward and we can reuse the formatter from other callers.


}

// Marshal the value
Expand Down
9 changes: 3 additions & 6 deletions plugins/shared/cmd/launcher/command/device.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"github.com/hashicorp/go-multierror"
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should appear in following grouping of imports, along with other non-stdlib imports. I'd suggest configuring your IDE to use goimports on save instead of plain gofmt.

"io/ioutil"
"os"
"os/exec"
Expand Down Expand Up @@ -196,13 +197,9 @@ func (c *Device) setConfig(spec hcldec.Spec, apiVersion string, config []byte, n

c.logger.Trace("raw hcl config", "config", hclog.Fmt("% #v", pretty.Formatter(configVal)))

val, diag := hclutils.ParseHclInterface(configVal, spec, nil)
val, diag, diagErrs := hclutils.ParseHclInterface(configVal, spec, nil)
if diag.HasErrors() {
errStr := "failed to parse config"
for _, err := range diag.Errs() {
errStr = fmt.Sprintf("%s\n* %s", errStr, err.Error())
}
return errors.New(errStr)
return multierror.Append(errors.New("failed to parse config"), diagErrs...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Of all of these, I like this previous formatting the most :P.

}
c.logger.Trace("parsed hcl config", "config", hclog.Fmt("% #v", pretty.Formatter(val)))

Expand Down