wip

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

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 {

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.

internal/helper/plugin/grpc_provider_test.go

@@ -1352,15 +1352,15 @@ 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() {
 					t.Fatal("expected error")
 				}
 			default:
 				if diags.HasErrors() {
-					t.Fatalf("unexpected error: %q", diags.Err())
+					t.Fatalf("unexpected error: %q", diags.Error())
 				}
 			}
 		})