From 30a28cca9430ce4c42ef5591ecd596ef284a2903 Mon Sep 17 00:00:00 2001 From: Alex Pilon Date: Thu, 26 Mar 2020 16:23:13 -0400 Subject: [PATCH] wip --- diag/diagnostic.go | 30 ++++++- helper/schema/resource.go | 92 ++++++++++++-------- internal/helper/plugin/grpc_provider.go | 14 +-- internal/helper/plugin/grpc_provider_test.go | 4 +- internal/plugin/convert/diagnostics.go | 90 +++++++++---------- internal/plugin/convert/diagnostics_test.go | 82 +++++------------ 6 files changed, 154 insertions(+), 158 deletions(-) diff --git a/diag/diagnostic.go b/diag/diagnostic.go index 2f50ecf074..2dc86c4872 100644 --- a/diag/diagnostic.go +++ b/diag/diagnostic.go @@ -3,11 +3,31 @@ package diag import ( "fmt" + "github.com/hashicorp/go-multierror" "github.com/zclconf/go-cty/cty" ) type Diagnostics []*Diagnostic +func (diags Diagnostics) Append(i interface{}) Diagnostics { + switch v := i.(type) { + case Diagnostics: + if len(v) != 0 { + diags = append(diags, v...) + } + case *Diagnostic: + diags = append(diags, v) + case error: + if v != nil { + diags = append(diags, &Diagnostic{ + Severity: Error, + Summary: v.Error(), + }) + } + } + return diags +} + func (diags Diagnostics) HasErrors() bool { for _, d := range diags { if d.Severity == Error { @@ -17,6 +37,10 @@ func (diags Diagnostics) HasErrors() bool { return false } +func (diags Diagnostics) Error() string { + return multierror.ListFormatFunc(diags.Errors()) +} + func (diags Diagnostics) Errors() []error { var errs []error for _, d := range diags { @@ -46,7 +70,11 @@ type Diagnostic struct { } func (d *Diagnostic) Error() string { - return fmt.Sprintf("%s: %s: %s", d.Severity, d.Summary, d.Detail) + s := fmt.Sprintf("%s: %s", d.Severity, d.Summary) + if d.Detail != "" { + s = fmt.Sprintf("%s: %s", s, d.Detail) + } + return s } type Severity int diff --git a/helper/schema/resource.go b/helper/schema/resource.go index 93cd5a1af3..eaaec9c673 100644 --- a/helper/schema/resource.go +++ b/helper/schema/resource.go @@ -265,49 +265,52 @@ type StateUpgradeFunc func(ctx context.Context, rawState map[string]interface{}, // See Resource documentation. type CustomizeDiffFunc func(context.Context, *ResourceDiff, interface{}) error -func (r *Resource) create(ctx context.Context, d *ResourceData, meta interface{}) error { +func (r *Resource) create(ctx context.Context, d *ResourceData, meta interface{}) (diags diag.Diagnostics) { if r.Create != nil { - return r.Create(d, meta) + return diags.Append(r.Create(d, meta)) } ctx, cancel := context.WithTimeout(ctx, d.Timeout(TimeoutCreate)) defer cancel() - return r.CreateContext(ctx, d, meta) + return diags.Append(r.CreateContext(ctx, d, meta)) } -func (r *Resource) read(ctx context.Context, d *ResourceData, meta interface{}) error { +func (r *Resource) read(ctx context.Context, d *ResourceData, meta interface{}) (diags diag.Diagnostics) { if r.Read != nil { - return r.Read(d, meta) + return diags.Append(r.Read(d, meta)) } ctx, cancel := context.WithTimeout(ctx, d.Timeout(TimeoutRead)) defer cancel() - return r.ReadContext(ctx, d, meta) + return diags.Append(r.ReadContext(ctx, d, meta)) } -func (r *Resource) update(ctx context.Context, d *ResourceData, meta interface{}) error { +func (r *Resource) update(ctx context.Context, d *ResourceData, meta interface{}) (diags diag.Diagnostics) { if r.Update != nil { - return r.Update(d, meta) + return diags.Append(r.Update(d, meta)) } ctx, cancel := context.WithTimeout(ctx, d.Timeout(TimeoutUpdate)) defer cancel() - return r.UpdateContext(ctx, d, meta) + return diags.Append(r.UpdateContext(ctx, d, meta)) } -func (r *Resource) delete(ctx context.Context, d *ResourceData, meta interface{}) error { +func (r *Resource) delete(ctx context.Context, d *ResourceData, meta interface{}) (diags diag.Diagnostics) { if r.Delete != nil { - return r.Delete(d, meta) + return diags.Append(r.Delete(d, meta)) } ctx, cancel := context.WithTimeout(ctx, d.Timeout(TimeoutDelete)) defer cancel() - return r.DeleteContext(ctx, d, meta) + return diags.Append(r.DeleteContext(ctx, d, meta)) } -func (r *Resource) exists(ctx context.Context, d *ResourceData, meta interface{}) (bool, error) { +func (r *Resource) exists(ctx context.Context, d *ResourceData, meta interface{}) (exists bool, diags diag.Diagnostics) { + var err error if r.Exists != nil { - return r.Exists(d, meta) + exists, err = r.Exists(d, meta) + return exists, diags.Append(err) } ctx, cancel := context.WithTimeout(ctx, d.Timeout(TimeoutRead)) defer cancel() - return r.ExistsContext(ctx, d, meta) + exists, err = r.ExistsContext(ctx, d, meta) + return exists, diags.Append(err) } // Apply creates, updates, and/or deletes a resource. @@ -316,21 +319,27 @@ func (r *Resource) Apply( s *terraform.InstanceState, d *terraform.InstanceDiff, meta interface{}) (*terraform.InstanceState, error) { + + var diags diag.Diagnostics + data, err := schemaMap(r.Schema).Data(s, d) - if err != nil { - return s, err + diags = diags.Append(err) + if diags.HasErrors() { + return s, diags } // Instance Diff shoould have the timeout info, need to copy it over to the // ResourceData meta rt := ResourceTimeout{} if _, ok := d.Meta[TimeoutKey]; ok { + // TODO should this be added to diags? if err := rt.DiffDecode(d); err != nil { log.Printf("[ERR] Error decoding ResourceTimeout: %s", err) } } else if s != nil { if _, ok := s.Meta[TimeoutKey]; ok { if err := rt.StateDecode(s); err != nil { + // TODO should this be added to diags? log.Printf("[ERR] Error decoding ResourceTimeout: %s", err) } } @@ -348,10 +357,10 @@ func (r *Resource) Apply( if d.Destroy || d.RequiresNew() { if s.ID != "" { // Destroy the resource since it is created - err := r.delete(ctx, data, meta) + diags = diags.Append(r.delete(ctx, data, meta)) - if err != nil { - return r.recordCurrentSchemaVersion(data.State()), err + if diags.HasErrors() { + return r.recordCurrentSchemaVersion(data.State()), diags } // Make sure the ID is gone. @@ -361,31 +370,31 @@ func (r *Resource) Apply( // If we're only destroying, and not creating, then return // now since we're done! if !d.RequiresNew() { - return nil, nil + return nil, diags } // Reset the data to be stateless since we just destroyed data, err = schemaMap(r.Schema).Data(nil, d) + diags = diags.Append(diags) // data was reset, need to re-apply the parsed timeouts data.timeouts = &rt - if err != nil { - return nil, err + if diags.HasErrors() { + return nil, diags } } - err = nil if data.Id() == "" { // We're creating, it is a new resource. data.MarkNewResource() - err = r.create(ctx, data, meta) + diags = diags.Append(r.create(ctx, data, meta)) } else { if !r.updateFuncSet() { return s, fmt.Errorf("doesn't support update") } - err = r.update(ctx, data, meta) + diags = diags.Append(r.update(ctx, data, meta)) } - return r.recordCurrentSchemaVersion(data.State()), err + return r.recordCurrentSchemaVersion(data.State()), diags } // Diff returns a diff of this resource. @@ -504,14 +513,18 @@ func (r *Resource) RefreshWithoutUpgrade( ctx context.Context, s *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) { + + var diags diag.Diagnostics + // If the ID is already somehow blank, it doesn't exist if s.ID == "" { - return nil, nil + return nil, diags } rt := ResourceTimeout{} if _, ok := s.Meta[TimeoutKey]; ok { if err := rt.StateDecode(s); err != nil { + // TODO should this be added to diags? log.Printf("[ERR] Error decoding ResourceTimeout: %s", err) } } @@ -520,36 +533,39 @@ func (r *Resource) RefreshWithoutUpgrade( // Make a copy of data so that if it is modified it doesn't // affect our Read later. data, err := schemaMap(r.Schema).Data(s, nil) + diags = diags.Append(err) data.timeouts = &rt - if err != nil { - return s, err + if diags.HasErrors() { + return s, diags } - exists, err := r.exists(ctx, data, meta) + exists, ds := r.exists(ctx, data, meta) + diags = diags.Append(ds) - if err != nil { - return s, err + if diags.HasErrors() { + return s, diags } if !exists { - return nil, nil + return nil, diags } } data, err := schemaMap(r.Schema).Data(s, nil) + diags = diags.Append(err) data.timeouts = &rt - if err != nil { - return s, err + if diags.HasErrors() { + return s, diags } - err = r.read(ctx, data, meta) + diags = diags.Append(r.read(ctx, data, meta)) state := data.State() if state != nil && state.ID == "" { state = nil } - return r.recordCurrentSchemaVersion(state), err + return r.recordCurrentSchemaVersion(state), diags } func (r *Resource) createFuncSet() bool { diff --git a/internal/helper/plugin/grpc_provider.go b/internal/helper/plugin/grpc_provider.go index 255a4522fe..7bda4a0f24 100644 --- a/internal/helper/plugin/grpc_provider.go +++ b/internal/helper/plugin/grpc_provider.go @@ -189,7 +189,7 @@ func (s *GRPCProviderServer) PrepareProviderConfig(_ context.Context, req *proto config := terraform.NewResourceConfigShimmed(configVal, schemaBlock) - resp.Diagnostics = convert.DiagsToProto(s.provider.ValidateDiag(config)) + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, s.provider.ValidateDiag(config)) preparedConfigMP, err := msgpack.Marshal(configVal, schemaBlock.ImpliedType()) if err != nil { @@ -215,7 +215,7 @@ func (s *GRPCProviderServer) ValidateResourceTypeConfig(_ context.Context, req * config := terraform.NewResourceConfigShimmed(configVal, schemaBlock) - resp.Diagnostics = convert.DiagsToProto(s.provider.ValidateResourceDiag(req.TypeName, config)) + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, s.provider.ValidateResourceDiag(req.TypeName, config)) return resp, nil } @@ -239,7 +239,7 @@ func (s *GRPCProviderServer) ValidateDataSourceConfig(_ context.Context, req *pr config := terraform.NewResourceConfigShimmed(configVal, schemaBlock) - resp.Diagnostics = convert.DiagsToProto(s.provider.ValidateDataSourceDiag(req.TypeName, config)) + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, s.provider.ValidateDataSourceDiag(req.TypeName, config)) return resp, nil } @@ -902,10 +902,10 @@ func (s *GRPCProviderServer) ApplyResourceChange(ctx context.Context, req *proto } newInstanceState, err := res.Apply(ctx, priorState, diff, s.provider.Meta()) - // we record the error here, but continue processing any returned state. - if err != nil { - resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) - } + // err could be an error or it could be diags. AppendProtoDiag will handle + // all the cases, if err is nil nothing is appended + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) + newStateVal := cty.NullVal(schemaBlock.ImpliedType()) // Always return a null value for destroy. diff --git a/internal/helper/plugin/grpc_provider_test.go b/internal/helper/plugin/grpc_provider_test.go index 8204a014be..f77178df8f 100644 --- a/internal/helper/plugin/grpc_provider_test.go +++ b/internal/helper/plugin/grpc_provider_test.go @@ -1352,7 +1352,7 @@ func TestValidateNulls(t *testing.T) { } { t.Run(strconv.Itoa(i), func(t *testing.T) { d := validateConfigNulls(tc.Cfg, nil) - diags := convert.ProtoToDiagnostics(d) + diags := convert.ProtoToDiags(d) switch { case tc.Err: if !diags.HasErrors() { @@ -1360,7 +1360,7 @@ func TestValidateNulls(t *testing.T) { } default: if diags.HasErrors() { - t.Fatalf("unexpected error: %q", diags.Err()) + t.Fatalf("unexpected error: %q", diags.Error()) } } }) diff --git a/internal/plugin/convert/diagnostics.go b/internal/plugin/convert/diagnostics.go index 80452915d0..b6878fd0d3 100644 --- a/internal/plugin/convert/diagnostics.go +++ b/internal/plugin/convert/diagnostics.go @@ -4,24 +4,9 @@ import ( "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" - "github.com/hashicorp/terraform-plugin-sdk/v2/internal/tfdiags" proto "github.com/hashicorp/terraform-plugin-sdk/v2/internal/tfplugin5" ) -// WarnsAndErrorsToProto converts the warnings and errors return by the legacy -// provider to protobuf diagnostics. -func WarnsAndErrsToProto(warns []string, errs []error) (diags []*proto.Diagnostic) { - for _, w := range warns { - diags = AppendProtoDiag(diags, w) - } - - for _, e := range errs { - diags = AppendProtoDiag(diags, e) - } - - return diags -} - // AppendProtoDiag appends a new diagnostic from a warning string or an error. // This panics if d is not a string or error. func AppendProtoDiag(diags []*proto.Diagnostic, d interface{}) []*proto.Diagnostic { @@ -33,11 +18,16 @@ func AppendProtoDiag(diags []*proto.Diagnostic, d interface{}) []*proto.Diagnost Summary: d.Error(), Attribute: ap, }) + // need to check Diagnostics before error since it implements error + case diag.Diagnostics: + diags = append(diags, DiagsToProto(d)...) case error: - diags = append(diags, &proto.Diagnostic{ - Severity: proto.Diagnostic_ERROR, - Summary: d.Error(), - }) + if d != nil { + diags = append(diags, &proto.Diagnostic{ + Severity: proto.Diagnostic_ERROR, + Summary: d.Error(), + }) + } case string: diags = append(diags, &proto.Diagnostic{ Severity: proto.Diagnostic_WARNING, @@ -51,35 +41,53 @@ func AppendProtoDiag(diags []*proto.Diagnostic, d interface{}) []*proto.Diagnost return diags } -// ProtoToDiagnostics converts a list of proto.Diagnostics to a tf.Diagnostics. -func ProtoToDiagnostics(ds []*proto.Diagnostic) tfdiags.Diagnostics { - var diags tfdiags.Diagnostics +// ProtoToDiagnostics converts a list of proto.Diagnostics to a diag.Diagnostics. +func ProtoToDiags(ds []*proto.Diagnostic) diag.Diagnostics { + var diags diag.Diagnostics for _, d := range ds { - var severity tfdiags.Severity + var severity diag.Severity switch d.Severity { case proto.Diagnostic_ERROR: - severity = tfdiags.Error + severity = diag.Error case proto.Diagnostic_WARNING: - severity = tfdiags.Warning + severity = diag.Warning } - var newDiag tfdiags.Diagnostic + newDiag := &diag.Diagnostic{ + Severity: severity, + Summary: d.Summary, + Detail: d.Detail, + } - // if there's an attribute path, we need to create a AttributeValue diagnostic if d.Attribute != nil { - path := AttributePathToPath(d.Attribute) - newDiag = tfdiags.AttributeValue(severity, d.Summary, d.Detail, path) - } else { - newDiag = tfdiags.WholeContainingBody(severity, d.Summary, d.Detail) + newDiag.AttributePath = AttributePathToPath(d.Attribute) } - diags = diags.Append(newDiag) + diags = append(diags, newDiag) } return diags } +func DiagsToProto(diags diag.Diagnostics) []*proto.Diagnostic { + var ds []*proto.Diagnostic + for _, d := range diags { + protoDiag := &proto.Diagnostic{ + Summary: d.Summary, + Detail: d.Detail, + Attribute: PathToAttributePath(d.AttributePath), + } + if d.Severity == diag.Error { + protoDiag.Severity = proto.Diagnostic_ERROR + } else if d.Severity == diag.Warning { + protoDiag.Severity = proto.Diagnostic_WARNING + } + ds = append(ds, protoDiag) + } + return ds +} + // AttributePathToPath takes the proto encoded path and converts it to a cty.Path func AttributePathToPath(ap *proto.AttributePath) cty.Path { var p cty.Path @@ -132,21 +140,3 @@ func PathToAttributePath(p cty.Path) *proto.AttributePath { } return ap } - -func DiagsToProto(diags diag.Diagnostics) []*proto.Diagnostic { - var ds []*proto.Diagnostic - for _, d := range diags { - protoDiag := &proto.Diagnostic{ - Summary: d.Summary, - Detail: d.Detail, - Attribute: PathToAttributePath(d.AttributePath), - } - if d.Severity == diag.Error { - protoDiag.Severity = proto.Diagnostic_ERROR - } else if d.Severity == diag.Warning { - protoDiag.Severity = proto.Diagnostic_WARNING - } - ds = append(ds, protoDiag) - } - return ds -} diff --git a/internal/plugin/convert/diagnostics_test.go b/internal/plugin/convert/diagnostics_test.go index 430cc0b19a..f0be6aa847 100644 --- a/internal/plugin/convert/diagnostics_test.go +++ b/internal/plugin/convert/diagnostics_test.go @@ -1,55 +1,18 @@ package convert import ( - "errors" "testing" "github.com/google/go-cmp/cmp" "github.com/zclconf/go-cty/cty" - "github.com/hashicorp/terraform-plugin-sdk/v2/internal/tfdiags" + "github.com/hashicorp/terraform-plugin-sdk/v2/diag" proto "github.com/hashicorp/terraform-plugin-sdk/v2/internal/tfplugin5" ) -func TestProtoDiagnostics(t *testing.T) { - diags := WarnsAndErrsToProto( - []string{ - "warning 1", - "warning 2", - }, - []error{ - errors.New("error 1"), - errors.New("error 2"), - }, - ) - - expected := []*proto.Diagnostic{ - { - Severity: proto.Diagnostic_WARNING, - Summary: "warning 1", - }, - { - Severity: proto.Diagnostic_WARNING, - Summary: "warning 2", - }, - { - Severity: proto.Diagnostic_ERROR, - Summary: "error 1", - }, - { - Severity: proto.Diagnostic_ERROR, - Summary: "error 2", - }, - } - - if !cmp.Equal(expected, diags) { - t.Fatal(cmp.Diff(expected, diags)) - } -} - func TestDiagnostics(t *testing.T) { type diagFlat struct { - Severity tfdiags.Severity + Severity diag.Severity Attr []interface{} Summary string Detail string @@ -74,7 +37,7 @@ func TestDiagnostics(t *testing.T) { }, []diagFlat{ { - Severity: tfdiags.Error, + Severity: diag.Error, Summary: "simple error", }, }, @@ -89,7 +52,7 @@ func TestDiagnostics(t *testing.T) { }, []diagFlat{ { - Severity: tfdiags.Error, + Severity: diag.Error, Summary: "simple error", Detail: "detailed error", }, @@ -104,7 +67,7 @@ func TestDiagnostics(t *testing.T) { }, []diagFlat{ { - Severity: tfdiags.Warning, + Severity: diag.Warning, Summary: "simple warning", }, }, @@ -119,7 +82,7 @@ func TestDiagnostics(t *testing.T) { }, []diagFlat{ { - Severity: tfdiags.Warning, + Severity: diag.Warning, Summary: "simple warning", Detail: "detailed warning", }, @@ -138,11 +101,11 @@ func TestDiagnostics(t *testing.T) { }, []diagFlat{ { - Severity: tfdiags.Error, + Severity: diag.Error, Summary: "first error", }, { - Severity: tfdiags.Error, + Severity: diag.Error, Summary: "second error", }, }, @@ -160,11 +123,11 @@ func TestDiagnostics(t *testing.T) { }, []diagFlat{ { - Severity: tfdiags.Warning, + Severity: diag.Warning, Summary: "warning", }, { - Severity: tfdiags.Error, + Severity: diag.Error, Summary: "error", }, }, @@ -189,7 +152,7 @@ func TestDiagnostics(t *testing.T) { }, []diagFlat{ { - Severity: tfdiags.Error, + Severity: diag.Error, Summary: "error", Detail: "error detail", Attr: []interface{}{"attribute_name"}, @@ -286,25 +249,25 @@ func TestDiagnostics(t *testing.T) { }, []diagFlat{ { - Severity: tfdiags.Error, + Severity: diag.Error, Summary: "error 1", Detail: "error 1 detail", Attr: []interface{}{"attr"}, }, { - Severity: tfdiags.Error, + Severity: diag.Error, Summary: "error 2", Detail: "error 2 detail", Attr: []interface{}{"attr", "sub"}, }, { - Severity: tfdiags.Warning, + Severity: diag.Warning, Summary: "warning", Detail: "warning detail", Attr: []interface{}{"attr", 1, "sub"}, }, { - Severity: tfdiags.Error, + Severity: diag.Error, Summary: "error 3", Detail: "error 3 detail", Attr: []interface{}{"attr", "idx", "sub"}, @@ -313,14 +276,13 @@ func TestDiagnostics(t *testing.T) { }, } - flattenTFDiags := func(ds tfdiags.Diagnostics) []diagFlat { + flattenDiags := func(ds diag.Diagnostics) []diagFlat { var flat []diagFlat for _, item := range ds { - desc := item.Description() var attr []interface{} - for _, a := range tfdiags.GetAttribute(item) { + for _, a := range item.AttributePath { switch step := a.(type) { case cty.GetAttrStep: attr = append(attr, step.Name) @@ -336,10 +298,10 @@ func TestDiagnostics(t *testing.T) { } flat = append(flat, diagFlat{ - Severity: item.Severity(), + Severity: item.Severity, Attr: attr, - Summary: desc.Summary, - Detail: desc.Detail, + Summary: item.Summary, + Detail: item.Detail, }) } return flat @@ -348,9 +310,9 @@ func TestDiagnostics(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { // we take the - tfDiags := ProtoToDiagnostics(tc.Cons(nil)) + diags := ProtoToDiags(tc.Cons(nil)) - flat := flattenTFDiags(tfDiags) + flat := flattenDiags(diags) if !cmp.Equal(flat, tc.Want, typeComparer, valueComparer, equateEmpty) { t.Fatal(cmp.Diff(flat, tc.Want, typeComparer, valueComparer, equateEmpty))