Skip to content

Commit

Permalink
Merge #20265: Don't presume unknown for values unset in config
Browse files Browse the repository at this point in the history
This changes the contract for `PlanResourceChange` so that the provider is now responsible
for populating all default values during plan, including inserting any unknown values for
defaults it will fill in at apply time.
  • Loading branch information
apparentlymart authored Feb 8, 2019
2 parents e677710 + c20164a commit 6eb7bfb
Show file tree
Hide file tree
Showing 20 changed files with 1,175 additions and 41 deletions.
4 changes: 2 additions & 2 deletions backend/local/backend_apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,7 @@ func TestLocal_applyError(t *testing.T) {
b, cleanup := TestLocal(t)
defer cleanup()

p := TestLocalProvider(t, b, "test", nil)
p.GetSchemaReturn = &terraform.ProviderSchema{
schema := &terraform.ProviderSchema{
ResourceTypes: map[string]*configschema.Block{
"test_instance": {
Attributes: map[string]*configschema.Attribute{
Expand All @@ -134,6 +133,7 @@ func TestLocal_applyError(t *testing.T) {
},
},
}
p := TestLocalProvider(t, b, "test", schema)

var lock sync.Mutex
errored := false
Expand Down
39 changes: 36 additions & 3 deletions backend/local/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import (
"path/filepath"
"testing"

"github.com/zclconf/go-cty/cty"

"github.com/hashicorp/terraform/addrs"
"github.com/hashicorp/terraform/backend"
"github.com/hashicorp/terraform/configs/configschema"
"github.com/hashicorp/terraform/providers"
"github.com/hashicorp/terraform/states"
"github.com/hashicorp/terraform/states/statemgr"
Expand Down Expand Up @@ -38,9 +42,14 @@ func TestLocal(t *testing.T) (*Local, func()) {
// function, t.Helper doesn't apply and so the log source
// isn't correctly shown in the test log output. This seems
// unavoidable as long as this is happening so indirectly.
t.Log(diag.Description().Summary)
desc := diag.Description()
if desc.Detail != "" {
t.Logf("%s: %s", desc.Summary, desc.Detail)
} else {
t.Log(desc.Summary)
}
if local.CLI != nil {
local.CLI.Error(diag.Description().Summary)
local.CLI.Error(desc.Summary)
}
}
}
Expand All @@ -59,10 +68,34 @@ func TestLocal(t *testing.T) (*Local, func()) {
func TestLocalProvider(t *testing.T, b *Local, name string, schema *terraform.ProviderSchema) *terraform.MockProvider {
// Build a mock resource provider for in-memory operations
p := new(terraform.MockProvider)

if schema == nil {
schema = &terraform.ProviderSchema{} // default schema is empty
}
p.GetSchemaReturn = schema

p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
rSchema, _ := schema.SchemaForResourceType(addrs.ManagedResourceMode, req.TypeName)
if rSchema == nil {
rSchema = &configschema.Block{} // default schema is empty
}
plannedVals := map[string]cty.Value{}
for name, attrS := range rSchema.Attributes {
val := req.ProposedNewState.GetAttr(name)
if attrS.Computed && val.IsNull() {
val = cty.UnknownVal(attrS.Type)
}
plannedVals[name] = val
}
for name := range rSchema.BlockTypes {
// For simplicity's sake we just copy the block attributes over
// verbatim, since this package's mock providers are all relatively
// simple -- we're testing the backend, not esoteric provider features.
plannedVals[name] = req.ProposedNewState.GetAttr(name)
}

return providers.PlanResourceChangeResponse{
PlannedState: req.ProposedNewState,
PlannedState: cty.ObjectVal(plannedVals),
PlannedPrivate: req.PriorPrivate,
}
}
Expand Down
55 changes: 55 additions & 0 deletions builtin/providers/test/resource_list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,58 @@ resource "test_resource_list" "foo" {
},
})
}

func TestResourceList_interpolationChanges(t *testing.T) {
resource.UnitTest(t, resource.TestCase{
Providers: testAccProviders,
CheckDestroy: testAccCheckResourceDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: strings.TrimSpace(`
resource "test_resource_list" "foo" {
list_block {
string = "x"
}
}
resource "test_resource_list" "bar" {
list_block {
string = test_resource_list.foo.id
}
}
`),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(
"test_resource_list.foo", "list_block.0.string", "x",
),
resource.TestCheckResourceAttr(
"test_resource_list.bar", "list_block.0.string", "testId",
),
),
},
resource.TestStep{
Config: strings.TrimSpace(`
resource "test_resource_list" "baz" {
list_block {
string = "x"
int = 1
}
}
resource "test_resource_list" "bar" {
list_block {
string = test_resource_list.baz.id
int = 3
}
}
`),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(
"test_resource_list.baz", "list_block.0.string", "x",
),
resource.TestCheckResourceAttr(
"test_resource_list.bar", "list_block.0.string", "testId",
),
),
},
},
})
}
53 changes: 53 additions & 0 deletions builtin/providers/test/resource_nested_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -527,3 +527,56 @@ resource "test_resource_nested_set" "c" {
},
})
}

func TestResourceNestedSet_interpolationChanges(t *testing.T) {
resource.UnitTest(t, resource.TestCase{
Providers: testAccProviders,
CheckDestroy: testAccCheckResourceDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: strings.TrimSpace(`
resource "test_resource_nested_set" "foo" {
single {
value = "x"
}
}
resource "test_resource_nested_set" "bar" {
single {
value = test_resource_nested_set.foo.id
}
}
`),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(
"test_resource_nested_set.foo", "single.#", "1",
),
resource.TestCheckResourceAttr(
"test_resource_nested_set.bar", "single.#", "1",
),
),
},
resource.TestStep{
Config: strings.TrimSpace(`
resource "test_resource_nested_set" "baz" {
single {
value = "x"
}
}
resource "test_resource_nested_set" "bar" {
single {
value = test_resource_nested_set.baz.id
}
}
`),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(
"test_resource_nested_set.baz", "single.#", "1",
),
resource.TestCheckResourceAttr(
"test_resource_nested_set.bar", "single.#", "1",
),
),
},
},
})
}
20 changes: 18 additions & 2 deletions command/show_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,13 +244,29 @@ func showFixtureProvider() *terraform.MockProvider {
p := testProvider()
p.GetSchemaReturn = showFixtureSchema()
p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
idVal := req.ProposedNewState.GetAttr("id")
amiVal := req.ProposedNewState.GetAttr("ami")
if idVal.IsNull() {
idVal = cty.UnknownVal(cty.String)
}
return providers.PlanResourceChangeResponse{
PlannedState: req.ProposedNewState,
PlannedState: cty.ObjectVal(map[string]cty.Value{
"id": idVal,
"ami": amiVal,
}),
}
}
p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse {
idVal := req.PlannedState.GetAttr("id")
amiVal := req.PlannedState.GetAttr("ami")
if !idVal.IsKnown() {
idVal = cty.StringVal("placeholder")
}
return providers.ApplyResourceChangeResponse{
NewState: cty.UnknownAsNull(req.PlannedState),
NewState: cty.ObjectVal(map[string]cty.Value{
"id": idVal,
"ami": amiVal,
}),
}
}
return p
Expand Down
8 changes: 4 additions & 4 deletions command/test-fixtures/show-json/basic-delete/output.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"schema_version": 0,
"values": {
"ami": "bar",
"id": null
"id": "placeholder"
}
}
]
Expand All @@ -37,11 +37,11 @@
],
"before": {
"ami": "foo",
"id": null
"id": "placeholder"
},
"after": {
"ami": "bar",
"id": null
"id": "placeholder"
},
"after_unknown": {
"ami": false,
Expand All @@ -61,7 +61,7 @@
],
"before": {
"ami": "foo",
"id": null
"id": "placeholder"
},
"after": null,
"after_unknown": false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
{
"schema_version": 0,
"attributes": {
"ami": "foo"
"ami": "foo",
"id": "placeholder"
}
}
]
Expand All @@ -28,7 +29,8 @@
{
"schema_version": 0,
"attributes": {
"ami": "foo"
"ami": "foo",
"id": "placeholder"
}
}
]
Expand Down
6 changes: 3 additions & 3 deletions command/test-fixtures/show-json/basic-update/output.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"schema_version": 0,
"values": {
"ami": "bar",
"id": null
"id": "placeholder"
}
}
]
Expand All @@ -37,11 +37,11 @@
],
"before": {
"ami": "bar",
"id": null
"id": "placeholder"
},
"after": {
"ami": "bar",
"id": null
"id": "placeholder"
},
"after_unknown": {
"ami": false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
{
"schema_version": 0,
"attributes": {
"ami": "bar"
"ami": "bar",
"id": "placeholder"
}
}
]
Expand Down
24 changes: 23 additions & 1 deletion configs/configschema/coerce_value.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,29 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) {
elems[key.AsString()] = val
}
}
attrs[typeName] = cty.MapVal(elems)

// If the attribute values here contain any DynamicPseudoTypes,
// the concrete type must be an object.
useObject := false
switch {
case coll.Type().IsObjectType():
useObject = true
default:
// It's possible that we were given a map, and need to coerce it to an object
ety := coll.Type().ElementType()
for _, v := range elems {
if !v.Type().Equals(ety) {
useObject = true
break
}
}
}

if useObject {
attrs[typeName] = cty.ObjectVal(elems)
} else {
attrs[typeName] = cty.MapVal(elems)
}
default:
attrs[typeName] = cty.MapValEmpty(blockS.ImpliedType())
}
Expand Down
Loading

0 comments on commit 6eb7bfb

Please sign in to comment.