Skip to content

Commit 9e15c51

Browse files
bfladbflad
authored
helper/schema: Update misleading error messaging during import (#878)
Reference: #877
1 parent 9f182f3 commit 9e15c51

File tree

2 files changed

+193
-72
lines changed

2 files changed

+193
-72
lines changed

‎helper/schema/provider.go‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -391,17 +391,32 @@ func (p *Provider) ImportState(
391391
// Convert the results to InstanceState values and return it
392392
states := make([]*terraform.InstanceState, len(results))
393393
for i, r := range results {
394+
if r == nil {
395+
return nil, fmt.Errorf("The provider returned a missing resource during ImportResourceState. " +
396+
"This is generally a bug in the resource implementation for import. " +
397+
"Resource import code should return an error for missing resources and skip returning a missing or empty ResourceData. " +
398+
"Please report this to the provider developers.")
399+
}
400+
401+
if r.Id() == "" {
402+
return nil, fmt.Errorf("The provider returned a resource missing an identifier during ImportResourceState. " +
403+
"This is generally a bug in the resource implementation for import. " +
404+
"Resource import code should not call d.SetId(\"\") or create an empty ResourceData. " +
405+
"If the resource is missing, instead return an error. " +
406+
"Please report this to the provider developers.")
407+
}
408+
394409
states[i] = r.State()
395410
}
396411

397412
// Verify that all are non-nil. If there are any nil the error
398413
// isn't obvious so we circumvent that with a friendlier error.
399414
for _, s := range states {
400415
if s == nil {
401-
return nil, fmt.Errorf(
402-
"nil entry in ImportState results. This is always a bug with\n" +
403-
"the resource that is being imported. Please report this as\n" +
404-
"a bug to Terraform.")
416+
return nil, fmt.Errorf("The provider returned a missing resource during ImportResourceState. " +
417+
"This is generally a bug in the resource implementation for import. " +
418+
"Resource import code should return an error for missing resources. " +
419+
"Please report this to the provider developers.")
405420
}
406421
}
407422

‎helper/schema/provider_test.go‎

Lines changed: 174 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -688,86 +688,192 @@ func TestProviderValidateResource(t *testing.T) {
688688
}
689689
}
690690

691-
func TestProviderImportState_default(t *testing.T) {
692-
p := &Provider{
693-
ResourcesMap: map[string]*Resource{
694-
"foo": {
695-
Importer: &ResourceImporter{},
691+
func TestProviderImportState(t *testing.T) {
692+
t.Parallel()
693+
694+
testCases := map[string]struct {
695+
provider *Provider
696+
info *terraform.InstanceInfo
697+
id string
698+
expectedStates []*terraform.InstanceState
699+
expectedErr error
700+
}{
701+
"error-unknown-resource-type": {
702+
provider: &Provider{
703+
ResourcesMap: map[string]*Resource{},
696704
},
705+
info: &terraform.InstanceInfo{
706+
Type: "test_resource",
707+
},
708+
id: "test-id",
709+
expectedErr: fmt.Errorf("unknown resource type: test_resource"),
697710
},
698-
}
699-
700-
states, err := p.ImportState(context.Background(), &terraform.InstanceInfo{
701-
Type: "foo",
702-
}, "bar")
703-
if err != nil {
704-
t.Fatalf("err: %s", err)
705-
}
706-
707-
if len(states) != 1 {
708-
t.Fatalf("bad: %#v", states)
709-
}
710-
if states[0].ID != "bar" {
711-
t.Fatalf("bad: %#v", states)
712-
}
713-
}
714-
715-
func TestProviderImportState_setsId(t *testing.T) {
716-
var val string
717-
stateFunc := func(d *ResourceData, meta interface{}) ([]*ResourceData, error) {
718-
val = d.Id()
719-
return []*ResourceData{d}, nil
720-
}
721-
722-
p := &Provider{
723-
ResourcesMap: map[string]*Resource{
724-
"foo": {
725-
Importer: &ResourceImporter{
726-
State: stateFunc,
711+
"error-no-Importer": {
712+
provider: &Provider{
713+
ResourcesMap: map[string]*Resource{
714+
"test_resource": { /* no Importer */ },
715+
},
716+
},
717+
info: &terraform.InstanceInfo{
718+
Type: "test_resource",
719+
},
720+
id: "test-id",
721+
expectedErr: fmt.Errorf("resource test_resource doesn't support import"),
722+
},
723+
"error-missing-ResourceData": {
724+
provider: &Provider{
725+
ResourcesMap: map[string]*Resource{
726+
"test_resource": {
727+
Importer: &ResourceImporter{
728+
StateContext: func(_ context.Context, _ *ResourceData, _ interface{}) ([]*ResourceData, error) {
729+
return []*ResourceData{nil}, nil
730+
},
731+
},
732+
},
733+
},
734+
},
735+
info: &terraform.InstanceInfo{
736+
Type: "test_resource",
737+
},
738+
id: "test-id",
739+
expectedErr: fmt.Errorf("The provider returned a missing resource during ImportResourceState."),
740+
},
741+
"error-missing-ResourceData-Id": {
742+
provider: &Provider{
743+
ResourcesMap: map[string]*Resource{
744+
"test_resource": {
745+
Importer: &ResourceImporter{
746+
StateContext: func(_ context.Context, d *ResourceData, _ interface{}) ([]*ResourceData, error) {
747+
// Example from calling Read functionality,
748+
// but not checking for missing resource before return
749+
d.SetId("")
750+
return []*ResourceData{d}, nil
751+
},
752+
},
753+
},
754+
},
755+
},
756+
info: &terraform.InstanceInfo{
757+
Type: "test_resource",
758+
},
759+
id: "test-id",
760+
expectedErr: fmt.Errorf("The provider returned a resource missing an identifier during ImportResourceState."),
761+
},
762+
"Importer": {
763+
provider: &Provider{
764+
ResourcesMap: map[string]*Resource{
765+
"test_resource": {
766+
Importer: &ResourceImporter{},
767+
},
768+
},
769+
},
770+
info: &terraform.InstanceInfo{
771+
Type: "test_resource",
772+
},
773+
id: "test-id",
774+
expectedStates: []*terraform.InstanceState{
775+
{
776+
Attributes: map[string]string{"id": "test-id"},
777+
Ephemeral: terraform.EphemeralState{Type: "test_resource"},
778+
ID: "test-id",
779+
Meta: map[string]interface{}{"schema_version": "0"},
780+
},
781+
},
782+
},
783+
"Importer-State": {
784+
provider: &Provider{
785+
ResourcesMap: map[string]*Resource{
786+
"test_resource": {
787+
Importer: &ResourceImporter{
788+
State: func(d *ResourceData, _ interface{}) ([]*ResourceData, error) {
789+
if d.Id() != "test-id" {
790+
return nil, fmt.Errorf("expected d.Id() %q, got: %s", "test-id", d.Id())
791+
}
792+
793+
if d.State().Ephemeral.Type != "test_resource" {
794+
return nil, fmt.Errorf("expected d.State().Ephemeral.Type %q, got: %s", "test_resource", d.State().Ephemeral.Type)
795+
}
796+
797+
return []*ResourceData{d}, nil
798+
},
799+
},
800+
},
801+
},
802+
},
803+
info: &terraform.InstanceInfo{
804+
Type: "test_resource",
805+
},
806+
id: "test-id",
807+
expectedStates: []*terraform.InstanceState{
808+
{
809+
Attributes: map[string]string{"id": "test-id"},
810+
Ephemeral: terraform.EphemeralState{Type: "test_resource"},
811+
ID: "test-id",
812+
Meta: map[string]interface{}{"schema_version": "0"},
813+
},
814+
},
815+
},
816+
"Importer-StateContext": {
817+
provider: &Provider{
818+
ResourcesMap: map[string]*Resource{
819+
"test_resource": {
820+
Importer: &ResourceImporter{
821+
StateContext: func(_ context.Context, d *ResourceData, meta interface{}) ([]*ResourceData, error) {
822+
if d.Id() != "test-id" {
823+
return nil, fmt.Errorf("expected d.Id() %q, got: %s", "test-id", d.Id())
824+
}
825+
826+
if d.State().Ephemeral.Type != "test_resource" {
827+
return nil, fmt.Errorf("expected d.State().Ephemeral.Type %q, got: %s", "test_resource", d.State().Ephemeral.Type)
828+
}
829+
830+
return []*ResourceData{d}, nil
831+
},
832+
},
833+
},
834+
},
835+
},
836+
info: &terraform.InstanceInfo{
837+
Type: "test_resource",
838+
},
839+
id: "test-id",
840+
expectedStates: []*terraform.InstanceState{
841+
{
842+
Attributes: map[string]string{"id": "test-id"},
843+
Ephemeral: terraform.EphemeralState{Type: "test_resource"},
844+
ID: "test-id",
845+
Meta: map[string]interface{}{"schema_version": "0"},
727846
},
728847
},
729848
},
730849
}
731850

732-
_, err := p.ImportState(context.Background(), &terraform.InstanceInfo{
733-
Type: "foo",
734-
}, "bar")
735-
if err != nil {
736-
t.Fatalf("err: %s", err)
737-
}
851+
for name, testCase := range testCases {
852+
name, testCase := name, testCase
738853

739-
if val != "bar" {
740-
t.Fatal("should set id")
741-
}
742-
}
854+
t.Run(name, func(t *testing.T) {
855+
t.Parallel()
743856

744-
func TestProviderImportState_setsType(t *testing.T) {
745-
var tVal string
746-
stateFunc := func(d *ResourceData, meta interface{}) ([]*ResourceData, error) {
747-
d.SetId("foo")
748-
tVal = d.State().Ephemeral.Type
749-
return []*ResourceData{d}, nil
750-
}
857+
states, err := testCase.provider.ImportState(context.Background(), testCase.info, testCase.id)
751858

752-
p := &Provider{
753-
ResourcesMap: map[string]*Resource{
754-
"foo": {
755-
Importer: &ResourceImporter{
756-
State: stateFunc,
757-
},
758-
},
759-
},
760-
}
859+
if err != nil {
860+
if testCase.expectedErr == nil {
861+
t.Fatalf("unexpected error: %s", err)
862+
}
761863

762-
_, err := p.ImportState(context.Background(), &terraform.InstanceInfo{
763-
Type: "foo",
764-
}, "bar")
765-
if err != nil {
766-
t.Fatalf("err: %s", err)
767-
}
864+
if !strings.Contains(err.Error(), testCase.expectedErr.Error()) {
865+
t.Fatalf("expected error %q, got: %s", testCase.expectedErr, err)
866+
}
867+
}
868+
869+
if err == nil && testCase.expectedErr != nil {
870+
t.Fatalf("expected error %q, got none", testCase.expectedErr)
871+
}
768872

769-
if tVal != "foo" {
770-
t.Fatal("should set type")
873+
if diff := cmp.Diff(states, testCase.expectedStates); diff != "" {
874+
t.Fatalf("unexpected states difference: %s", diff)
875+
}
876+
})
771877
}
772878
}
773879

0 commit comments

Comments
 (0)