r/heroku_app: always read all config vars

[catsby]

The current implementation of resource_heroku_app only reads values for configuration variables for values defined in the current Terraform configuration files. This is counter to how Terraform Resources generally work. Unless there is an extreme circumstance, Terraform Resources should read all supported values regardless if they are set locally, so that a user can detect configuration drift.

Consider this config:

provider "heroku" {}

resource "heroku_app" "test_config_vars" {
  name   = "test-config-vars"
  region = "us"

  config_vars {
    OTHER_TOKEN = "twothree4"
    LAST_TOKEN  = "threefour5"
    EDIT        = "ok"
    NEW         = "best"
    MORE        = "another"
  }
}

If another user were to add additional configuration variables via the Web or API outside of Terraform, the current implementation would not detect it. I found this by importing an app with vars and none in my config, and no diff was detected.

In this patch, we read and save all configuration values. The all_config_vars (?) attribute should be removed in a future release.

Test results after the patch:

TF_ACC=1 go test ./heroku -v -run=TestAccHerokuApp -timeout 120m
=== RUN   TestAccHerokuApp_importBasic
--- PASS: TestAccHerokuApp_importBasic (2.68s)
=== RUN   TestAccHerokuApp_importOrganization
--- PASS: TestAccHerokuApp_importOrganization (2.62s)
=== RUN   TestAccHerokuAppFeature
--- PASS: TestAccHerokuAppFeature (4.52s)
=== RUN   TestAccHerokuApp_Basic
--- PASS: TestAccHerokuApp_Basic (2.33s)
=== RUN   TestAccHerokuApp_NameChange
--- PASS: TestAccHerokuApp_NameChange (4.12s)
=== RUN   TestAccHerokuApp_NukeVars
--- PASS: TestAccHerokuApp_NukeVars (3.61s)
=== RUN   TestAccHerokuApp_Buildpacks
--- PASS: TestAccHerokuApp_Buildpacks (5.53s)
=== RUN   TestAccHerokuApp_ExternallySetBuildpacks
--- PASS: TestAccHerokuApp_ExternallySetBuildpacks (3.28s)
=== RUN   TestAccHerokuApp_Organization
--- PASS: TestAccHerokuApp_Organization (2.20s)
=== RUN   TestAccHerokuApp_Space
--- PASS: TestAccHerokuApp_Space (420.77s)
=== RUN   TestAccHerokuApp_EmptyConfigVars
--- PASS: TestAccHerokuApp_EmptyConfigVars (1.94s)
PASS
ok      github.com/terraform-providers/terraform-provider-heroku/heroku 453.635s

[danp]

Related to hashicorp/terraform#15962?

[catsby]

@danp they appear to be unrelated; my change still gives that weird diff. The diff presented in that issue is... misleading? The net result is that var FOO is still there with that value.

[radeksimko]

This generally makes sense to me with the explanation you provided.

I would question why the field was added in the first place, but all I found is this commit and you know Heroku a lot better than me 😄 , so I will assume you know what you're doing.

I know in K8S it's common (expected) practice for the kubelet to mess up with annotations + let users specify their own, which is (unfortunately) probably going to force me to do the exact opposite: hashicorp/terraform-provider-kubernetes#60

[radeksimko]

oh btw. keep in mind that we don't print out deprecation warnings for computed-only field, so this won't have any effect in the UI.

hashicorp/terraform#7569

[catsby]

I'm going to go ahead and merge this on Friday, November 3rd, if I don't hear otherwise before then. Thanks!