From 73ab98db7c96216856bdb483d6e73a85e1bde958 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Tue, 15 Mar 2016 10:03:01 -0500 Subject: [PATCH] core: Don't filter ignore_changes on create The ignore_changes diff filter was stripping out attributes on Create but the diff was still making it down to the provider, so Create would end up missing attributes, causing a full failure if any required attributes were being ignored. This should fix `ignore_changes` problems with Required attributes. Here we also introduce a `test` provider meant as an aid to exposing via automated tests issues involving interactions between `helper/schema` and Terraform core. This has been helpful so far in diagnosing `ignore_changes` problems, and I imagine it will be helpful in other contexts as well. We'll have to be careful to prevent the `test` provider from becoming a dumping ground for poorly specified tests that have a clear home elsewhere. But for bug exposure I think it's useful to have. Refs #5627 --- builtin/providers/test/provider.go | 19 +++++++ builtin/providers/test/provider_test.go | 16 ++++++ builtin/providers/test/resource.go | 53 ++++++++++++++++++ builtin/providers/test/resource_test.go | 54 +++++++++++++++++++ helper/resource/testing.go | 16 ++++++ terraform/context_apply_test.go | 41 ++++++++++++++ terraform/eval_ignore_changes.go | 6 +++ .../apply-ignore-changes-create/main.tf | 7 +++ 8 files changed, 212 insertions(+) create mode 100644 builtin/providers/test/provider.go create mode 100644 builtin/providers/test/provider_test.go create mode 100644 builtin/providers/test/resource.go create mode 100644 builtin/providers/test/resource_test.go create mode 100644 terraform/test-fixtures/apply-ignore-changes-create/main.tf diff --git a/builtin/providers/test/provider.go b/builtin/providers/test/provider.go new file mode 100644 index 000000000000..a8e8c2470a59 --- /dev/null +++ b/builtin/providers/test/provider.go @@ -0,0 +1,19 @@ +package test + +import ( + "github.com/hashicorp/terraform/helper/schema" + "github.com/hashicorp/terraform/terraform" +) + +func Provider() terraform.ResourceProvider { + return &schema.Provider{ + ResourcesMap: map[string]*schema.Resource{ + "test_resource": testResource(), + }, + ConfigureFunc: providerConfigure, + } +} + +func providerConfigure(d *schema.ResourceData) (interface{}, error) { + return nil, nil +} diff --git a/builtin/providers/test/provider_test.go b/builtin/providers/test/provider_test.go new file mode 100644 index 000000000000..680bff119652 --- /dev/null +++ b/builtin/providers/test/provider_test.go @@ -0,0 +1,16 @@ +package test + +import ( + "github.com/hashicorp/terraform/helper/schema" + "github.com/hashicorp/terraform/terraform" +) + +var testAccProviders map[string]terraform.ResourceProvider +var testAccProvider *schema.Provider + +func init() { + testAccProvider = Provider().(*schema.Provider) + testAccProviders = map[string]terraform.ResourceProvider{ + "test": testAccProvider, + } +} diff --git a/builtin/providers/test/resource.go b/builtin/providers/test/resource.go new file mode 100644 index 000000000000..93403ea92f1a --- /dev/null +++ b/builtin/providers/test/resource.go @@ -0,0 +1,53 @@ +package test + +import ( + "fmt" + + "github.com/hashicorp/terraform/helper/schema" +) + +func testResource() *schema.Resource { + return &schema.Resource{ + Create: testResourceCreate, + Read: testResourceRead, + Update: testResourceUpdate, + Delete: testResourceDelete, + Schema: map[string]*schema.Schema{ + "required": &schema.Schema{ + Type: schema.TypeString, + Required: true, + }, + "optional": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + }, + "optional_force_new": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + ForceNew: true, + }, + }, + } +} + +func testResourceCreate(d *schema.ResourceData, meta interface{}) error { + d.SetId("testId") + + // Required must make it through to Create + if _, ok := d.GetOk("required"); !ok { + return fmt.Errorf("Missing attribute 'required', but it's required!") + } + return nil +} + +func testResourceRead(d *schema.ResourceData, meta interface{}) error { + return nil +} + +func testResourceUpdate(d *schema.ResourceData, meta interface{}) error { + return nil +} + +func testResourceDelete(d *schema.ResourceData, meta interface{}) error { + return nil +} diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go new file mode 100644 index 000000000000..8cdb70ddd3bb --- /dev/null +++ b/builtin/providers/test/resource_test.go @@ -0,0 +1,54 @@ +package test + +import ( + "strings" + "testing" + + "github.com/hashicorp/terraform/helper/resource" + "github.com/hashicorp/terraform/terraform" +) + +func TestResource_basic(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + }, + }) +} + +func TestResource_ignoreChangesRequired(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + lifecycle { + ignore_changes = ["required"] + } +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + }, + }) +} + +func testAccCheckResourceDestroy(s *terraform.State) error { + return nil +} diff --git a/helper/resource/testing.go b/helper/resource/testing.go index c2e50bdebba2..80c56d2e968f 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -173,6 +173,22 @@ func Test(t TestT, c TestCase) { } } +// UnitTest is a helper to force the acceptance testing harness to run in the +// normal unit test suite. This should only be used for resource that don't +// have any external dependencies. +func UnitTest(t TestT, c TestCase) { + oldEnv := os.Getenv(TestEnvVar) + if err := os.Setenv(TestEnvVar, "UnitTestOverride"); err != nil { + t.Fatal(err) + } + defer func() { + if err := os.Setenv(TestEnvVar, oldEnv); err != nil { + t.Fatal(err) + } + }() + Test(t, c) +} + func testStep( opts terraform.ContextOpts, state *terraform.State, diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index a815f3928aaa..361f27379af0 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -4084,3 +4084,44 @@ aws_instance.ifailedprovisioners: (1 tainted) t.Fatalf("expected state: \n%s\ngot: \n%s", expected, actual) } } + +func TestContext2Apply_ignoreChangesCreate(t *testing.T) { + m := testModule(t, "apply-ignore-changes-create") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + if p, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } else { + t.Logf(p.String()) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + mod := state.RootModule() + if len(mod.Resources) != 1 { + t.Fatalf("bad: %s", state) + } + + actual := strings.TrimSpace(state.String()) + // Expect no changes from original state + expected := strings.TrimSpace(` +aws_instance.foo: + ID = foo + required_field = set + type = aws_instance +`) + if actual != expected { + t.Fatalf("bad: \n%s", actual) + } +} diff --git a/terraform/eval_ignore_changes.go b/terraform/eval_ignore_changes.go index cc2261313714..3ae1d4995f19 100644 --- a/terraform/eval_ignore_changes.go +++ b/terraform/eval_ignore_changes.go @@ -22,6 +22,12 @@ func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) { diff := *n.Diff ignoreChanges := n.Resource.Lifecycle.IgnoreChanges + // If we're just creating the resource, we shouldn't alter the + // Diff at all + if diff.ChangeType() == DiffCreate { + return nil, nil + } + for _, ignoredName := range ignoreChanges { for name := range diff.Attributes { if strings.HasPrefix(name, ignoredName) { diff --git a/terraform/test-fixtures/apply-ignore-changes-create/main.tf b/terraform/test-fixtures/apply-ignore-changes-create/main.tf new file mode 100644 index 000000000000..d470660ec1cc --- /dev/null +++ b/terraform/test-fixtures/apply-ignore-changes-create/main.tf @@ -0,0 +1,7 @@ +resource "aws_instance" "foo" { + required_field = "set" + + lifecycle { + ignore_changes = ["required_field"] + } +}