resource ids must always have a value

The "id" field is assumed to always exist, and must have a valid value.
Set "id" to unknown when planning new resource changes to indicate that
it will be computed.
This commit is contained in:
James Bardin 2018-09-04 15:03:44 -04:00 committed by Martin Atkins
parent e6c958048c
commit a87470cc15
2 changed files with 77 additions and 4 deletions

View File

@ -55,12 +55,20 @@ func ApplyDiff(state cty.Value, d *terraform.InstanceDiff, schemaBlock *configsc
if d.Destroy || d.DestroyDeposed || d.DestroyTainted { if d.Destroy || d.DestroyDeposed || d.DestroyTainted {
// to mark a destroy, we remove all attributes // to mark a destroy, we remove all attributes
attrs = map[string]string{} attrs = map[string]string{}
} else if attrs["id"] == "" || d.RequiresNew() {
// Since "id" is always computed, make sure it always has a value. Set
// it as unknown to generate the correct cty.Value
attrs["id"] = config.UnknownVariableValue
} }
for attr, diff := range d.Attributes { for attr, diff := range d.Attributes {
old, exists := attrs[attr] old, exists := attrs[attr]
if old != diff.Old && exists { if exists &&
old != diff.Old &&
// if new or old is unknown, then there's no mismatch
old != config.UnknownVariableValue &&
diff.Old != config.UnknownVariableValue {
return state, fmt.Errorf("mismatched diff: %q != %q", old, diff.Old) return state, fmt.Errorf("mismatched diff: %q != %q", old, diff.Old)
} }

View File

@ -32,10 +32,9 @@ func testApplyDiff(t *testing.T,
resource *Resource, resource *Resource,
state, expected *terraform.InstanceState, state, expected *terraform.InstanceState,
diff *terraform.InstanceDiff) { diff *terraform.InstanceDiff) {
t.Helper()
testSchema := providers.Schema{ testSchema := providers.Schema{
Version: resource.SchemaVersion, Version: uint64(resource.SchemaVersion),
Block: resourceSchemaToBlock(resource.Schema), Block: resourceSchemaToBlock(resource.Schema),
} }
@ -49,6 +48,27 @@ func testApplyDiff(t *testing.T,
t.Fatal(err) t.Fatal(err)
} }
// verify that "id" is correct
id := newState.AsValueMap()["id"]
switch {
case diff.Destroy || diff.DestroyDeposed || diff.DestroyTainted:
// there should be no id
if !id.IsNull() {
t.Fatalf("destroyed instance should have no id: %#v", id)
}
default:
// the "id" field always exists and is computed, so it must have a
// valid value or be unknown.
if id.IsNull() {
t.Fatal("new instance state cannot have a null id")
}
if id.IsKnown() && id.AsString() == "" {
t.Fatal("new instance id cannot be an empty string")
}
}
// Resource.Meta will be hanlded separately, so it's OK that we lose the // Resource.Meta will be hanlded separately, so it's OK that we lose the
// timeout values here. // timeout values here.
expectedState, err := StateValueFromInstanceState(expected, testSchema.Block.ImpliedType()) expectedState, err := StateValueFromInstanceState(expected, testSchema.Block.ImpliedType())
@ -61,6 +81,46 @@ func testApplyDiff(t *testing.T,
} }
} }
func TestShimResourcePlan_destroyCreate(t *testing.T) {
r := &Resource{
SchemaVersion: 2,
Schema: map[string]*Schema{
"foo": &Schema{
Type: TypeInt,
Optional: true,
ForceNew: true,
},
},
}
d := &terraform.InstanceDiff{
Attributes: map[string]*terraform.ResourceAttrDiff{
"foo": &terraform.ResourceAttrDiff{
RequiresNew: true,
Old: "3",
New: "42",
},
},
}
state := &terraform.InstanceState{
Attributes: map[string]string{"foo": "3"},
}
expected := &terraform.InstanceState{
ID: config.UnknownVariableValue,
Attributes: map[string]string{
"id": config.UnknownVariableValue,
"foo": "42",
},
Meta: map[string]interface{}{
"schema_version": "2",
},
}
testApplyDiff(t, r, state, expected, d)
}
func TestShimResourceApply_create(t *testing.T) { func TestShimResourceApply_create(t *testing.T) {
r := &Resource{ r := &Resource{
SchemaVersion: 2, SchemaVersion: 2,
@ -279,7 +339,7 @@ func TestShimResourceDiff_Timeout_diff(t *testing.T) {
} }
testSchema := providers.Schema{ testSchema := providers.Schema{
Version: r.SchemaVersion, Version: uint64(r.SchemaVersion),
Block: resourceSchemaToBlock(r.Schema), Block: resourceSchemaToBlock(r.Schema),
} }
@ -351,6 +411,7 @@ func TestShimResourceApply_destroyCreate(t *testing.T) {
"foo": &Schema{ "foo": &Schema{
Type: TypeInt, Type: TypeInt,
Optional: true, Optional: true,
ForceNew: true,
}, },
"tags": &Schema{ "tags": &Schema{
@ -381,6 +442,9 @@ func TestShimResourceApply_destroyCreate(t *testing.T) {
d := &terraform.InstanceDiff{ d := &terraform.InstanceDiff{
Attributes: map[string]*terraform.ResourceAttrDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{
"id": &terraform.ResourceAttrDiff{
New: "foo",
},
"foo": &terraform.ResourceAttrDiff{ "foo": &terraform.ResourceAttrDiff{
Old: "7", Old: "7",
New: "42", New: "42",
@ -424,6 +488,7 @@ func TestShimResourceApply_destroyCreate(t *testing.T) {
createdState := &terraform.InstanceState{ createdState := &terraform.InstanceState{
ID: "foo", ID: "foo",
Attributes: map[string]string{ Attributes: map[string]string{
"id": "foo",
"foo": "7", "foo": "7",
"tags.%": "1", "tags.%": "1",
"tags.Name": "foo", "tags.Name": "foo",