Merge pull request #379 from hashicorp/deprecate-exists

Deprecate Exists

Author: appilon

helper/resource/state.go

@@ -276,5 +276,6 @@ func (conf *StateChangeConf) WaitForStateContext(ctx context.Context) (interface
 //
 // Deprecated: Please use WaitForStateContext to ensure proper plugin shutdown
 func (conf *StateChangeConf) WaitForState() (interface{}, error) {
+	log.Printf("[WARN] WaitForState is deprecated, please use WaitForStateContext")
 	return conf.WaitForStateContext(context.Background())
 }

helper/resource/wait.go

@@ -3,6 +3,7 @@ package resource
 import (
 	"context"
 	"errors"
+	"log"
 	"sync"
 	"time"
 )
@@ -65,6 +66,7 @@ func RetryContext(ctx context.Context, timeout time.Duration, f RetryFunc) error
 //
 // Deprecated: Please use RetryContext to ensure proper plugin shutdown
 func Retry(timeout time.Duration, f RetryFunc) error {
+	log.Printf("[WARN] Retry is deprecated, please use RetryContext")
 	return RetryContext(context.Background(), timeout, f)
 }
 

helper/schema/provider.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"log"
 	"sort"
 
 	"github.com/hashicorp/go-multierror"
@@ -89,6 +90,10 @@ func (p *Provider) InternalValidate() error {
 		return errors.New("provider is nil")
 	}
 
+	if p.ConfigureFunc != nil && p.ConfigureContextFunc != nil {
+		return errors.New("ConfigureFunc and ConfigureContextFunc must not both be set")
+	}
+
 	var validationErrors error
 	sm := schemaMap(p.Schema)
 	if err := sm.InternalValidate(sm); err != nil {
@@ -114,6 +119,11 @@ func (p *Provider) InternalValidate() error {
 		}
 	}
 
+	// Warn of deprecations
+	if p.ConfigureFunc != nil && p.ConfigureContextFunc == nil {
+		log.Printf("[WARN] ConfigureFunc is deprecated, please use ConfigureContextFunc")
+	}
+
 	return validationErrors
 }
 

helper/schema/provider_test.go

@@ -540,6 +540,23 @@ func TestProvider_InternalValidate(t *testing.T) {
 			},
 			ExpectedErr: fmt.Errorf("%s is a reserved field name for a provider", "alias"),
 		},
+		{ // ConfigureFunc and ConfigureContext cannot both be set
+			P: &Provider{
+				Schema: map[string]*Schema{
+					"foo": {
+						Type:     TypeString,
+						Optional: true,
+					},
+				},
+				ConfigureFunc: func(d *ResourceData) (interface{}, error) {
+					return nil, nil
+				},
+				ConfigureContextFunc: func(ctx context.Context, d *ResourceData) (interface{}, error) {
+					return nil, nil
+				},
+			},
+			ExpectedErr: fmt.Errorf("ConfigureFunc and ConfigureContextFunc must not both be set"),
+		},
 	}
 
 	for i, tc := range cases {

helper/schema/resource.go

@@ -103,11 +103,20 @@ type Resource struct {
 	Read   ReadFunc
 	Update UpdateFunc
 	Delete DeleteFunc
+
+	// Exists is a function that is called to check if a resource still
+	// exists. If this returns false, then this will affect the diff
+	// accordingly. If this function isn't set, it will not be called. You
+	// can also signal existence in the Read method by calling d.SetId("")
+	// if the Resource is no longer present and should be removed from state.
+	// The *ResourceData passed to Exists should _not_ be modified.
+	//
+	// Deprecated: ReadContext should be able to encapsulate the logic of Exists
 	Exists ExistsFunc
 
 	// The functions below are the CRUD operations for this resource.
 	//
-	// The only optional operation is Update and Exists. If Update is not
+	// The only optional operation is Update. If Update is not
 	// implemented, then updates will not be supported for this resource.
 	//
 	// The ResourceData parameter in the functions below are used to
@@ -120,20 +129,11 @@ type Resource struct {
 	// to store API clients, configuration structures, etc.
 	//
 	// If any errors occur during each of the operation, an error should be
-	// returned. If a resource was partially updated, be careful to enable
-	// partial state mode for ResourceData and use it accordingly.
-	//
-	// Exists is a function that is called to check if a resource still
-	// exists. If this returns false, then this will affect the diff
-	// accordingly. If this function isn't set, it will not be called. You
-	// can also signal existence in the Read method by calling d.SetId("")
-	// if the Resource is no longer present and should be removed from state.
-	// The *ResourceData passed to Exists should _not_ be modified.
+	// returned.
 	CreateContext CreateContextFunc
 	ReadContext   ReadContextFunc
 	UpdateContext UpdateContextFunc
 	DeleteContext DeleteContextFunc
-	ExistsContext ExistsContextFunc
 
 	// CustomizeDiff is a custom function for working with the diff that
 	// Terraform has created for this resource - it can be used to customize the
@@ -233,9 +233,6 @@ type UpdateContextFunc func(context.Context, *ResourceData, interface{}) error
 // See Resource documentation.
 type DeleteContextFunc func(context.Context, *ResourceData, interface{}) error
 
-// See Resource documentation.
-type ExistsContextFunc func(context.Context, *ResourceData, interface{}) (bool, error)
-
 // See Resource documentation.
 type StateMigrateFunc func(
 	int, *terraform.InstanceState, interface{}) (*terraform.InstanceState, error)
@@ -300,15 +297,6 @@ func (r *Resource) delete(ctx context.Context, d *ResourceData, meta interface{}
 	return r.DeleteContext(ctx, d, meta)
 }
 
-func (r *Resource) exists(ctx context.Context, d *ResourceData, meta interface{}) (bool, error) {
-	if r.Exists != nil {
-		return r.Exists(d, meta)
-	}
-	ctx, cancel := context.WithTimeout(ctx, d.Timeout(TimeoutRead))
-	defer cancel()
-	return r.ExistsContext(ctx, d, meta)
-}
-
 // Apply creates, updates, and/or deletes a resource.
 func (r *Resource) Apply(
 	ctx context.Context,
@@ -505,7 +493,7 @@ func (r *Resource) RefreshWithoutUpgrade(
 		}
 	}
 
-	if r.existsFuncSet() {
+	if r.Exists != nil {
 		// 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)
@@ -515,7 +503,7 @@ func (r *Resource) RefreshWithoutUpgrade(
 			return s, err
 		}
 
-		exists, err := r.exists(ctx, data, meta)
+		exists, err := r.Exists(data, meta)
 
 		if err != nil {
 			return s, err
@@ -557,10 +545,6 @@ func (r *Resource) deleteFuncSet() bool {
 	return (r.Delete != nil || r.DeleteContext != nil)
 }
 
-func (r *Resource) existsFuncSet() bool {
-	return (r.Exists != nil || r.ExistsContext != nil)
-}
-
 // InternalValidate should be called to validate the structure
 // of the resource.
 //
@@ -688,8 +672,25 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap, writable bool) error
 	if r.DeleteContext != nil && r.Delete != nil {
 		return fmt.Errorf("DeleteContext and Delete should not both be set")
 	}
-	if r.ExistsContext != nil && r.Exists != nil {
-		return fmt.Errorf("ExistsContext and Exists should not both be set")
+
+	// Warn of deprecations
+	if r.Exists != nil {
+		log.Printf("[WARN] Exists is deprecated, please encapsulate the logic in ReadContext")
+	}
+	if r.Create != nil && r.CreateContext == nil {
+		log.Printf("[WARN] Create is deprecated, please use CreateContext")
+	}
+	if r.Read != nil && r.ReadContext == nil {
+		log.Printf("[WARN] Read is deprecated, please use ReadContext")
+	}
+	if r.Update != nil && r.UpdateContext == nil {
+		log.Printf("[WARN] Update is deprecated, please use UpdateContext")
+	}
+	if r.Delete != nil && r.DeleteContext == nil {
+		log.Printf("[WARN] Delete is deprecated, please use DeleteContext")
+	}
+	if r.MigrateState != nil {
+		log.Printf("[WARN] MigrateState is deprecated, please use StateUpgraders")
 	}
 
 	return schemaMap(r.Schema).InternalValidate(tsm)

helper/schema/resource_importer.go

@@ -3,6 +3,7 @@ package schema
 import (
 	"context"
 	"errors"
+	"log"
 )
 
 // ResourceImporter defines how a resource is imported in Terraform. This
@@ -59,6 +60,9 @@ func (r *ResourceImporter) InternalValidate() error {
 	if r.State != nil && r.StateContext != nil {
 		return errors.New("Both State and StateContext cannot be set.")
 	}
+	if r.State != nil && r.StateContext == nil {
+		log.Printf("[WARN] State is deprecated, please use StateContext")
+	}
 	return nil
 }
 
@@ -67,6 +71,7 @@ func (r *ResourceImporter) InternalValidate() error {
 //
 // Deprecated: Please use the context aware ImportStatePassthroughContext instead
 func ImportStatePassthrough(d *ResourceData, m interface{}) ([]*ResourceData, error) {
+	log.Printf("[WARN] ImportStatePassthrough is deprecated, please use ImportStatePassthroughContext")
 	return []*ResourceData{d}, nil
 }
 

helper/schema/resource_importer_test.go

@@ -0,0 +1,13 @@
+package schema
+
+import "testing"
+
+func TestInternalValidate(t *testing.T) {
+	r := &ResourceImporter{
+		State:        ImportStatePassthrough,
+		StateContext: ImportStatePassthroughContext,
+	}
+	if err := r.InternalValidate(); err == nil {
+		t.Fatal("ResourceImporter should not allow State and StateContext to be set")
+	}
+}

helper/schema/resource_test.go

@@ -928,24 +928,6 @@ func TestResourceInternalValidate(t *testing.T) {
 			true,
 			true,
 		},
-		19: { // Exists and ExistsContext should not both be set
-			&Resource{
-				Create:        Noop,
-				Read:          Noop,
-				Update:        Noop,
-				Delete:        Noop,
-				Exists:        func(*ResourceData, interface{}) (bool, error) { return false, nil },
-				ExistsContext: func(context.Context, *ResourceData, interface{}) (bool, error) { return false, nil },
-				Schema: map[string]*Schema{
-					"foo": {
-						Type:     TypeString,
-						Required: true,
-					},
-				},
-			},
-			true,
-			true,
-		},
 	}
 
 	for i, tc := range cases {

internal/helper/plugin/grpc_provider.go

@@ -360,7 +360,6 @@ func (s *GRPCProviderServer) upgradeFlatmapState(ctx context.Context, version in
 				"schema_version": strconv.Itoa(version),
 			},
 		}
-
 		is, err := res.MigrateState(version, is, s.provider.Meta())
 		if err != nil {
 			return nil, 0, err