Config var data #109

shirdoo · 2018-08-14T21:01:36Z

No description provided.

talbright · 2018-08-14T21:04:08Z

Don't forget to update the docs.

shirdoo · 2018-08-14T21:05:50Z

heroku/data_source_heroku_app_config_vars_test.go

+		Steps: []resource.TestStep{
+			{
+				Config:             testAccCheckHerokuAppConfigVar(appName),
+				ExpectNonEmptyPlan: true, // todo: why do we get non-empty plans after an apply.... boo


I'm bummed I had to add this, and I don't know why (advice appreciated). Without this line, this test fails with:

=== RUN TestAccDatasourceHerokuAppConfigVars --- FAIL: TestAccDatasourceHerokuAppConfigVars (11.15s) /Users/sbabu/go/src/github.com/terraform-providers/terraform-provider-heroku/heroku/testing.go:518: Step 0 error: After applying this step and refreshing, the plan was not empty: DIFF: CREATE: data.heroku_app_config_vars.app_config_vars all_config_vars.%: "" => "<computed>" app: "" => "tftest-app-iycoxl7pzx" UPDATE: heroku_app.app2 config_vars.0.%: "" => "<computed>" config_vars.0.HOSTEDGRAPHITE_APIKEY: "25da0edc-c571-4664-b578-cae2b6b040e7" => "" STATE: heroku_addon.hostedgraphite: ID = f7666c96-acf5-44d2-9fa5-e97e5ddac1cd provider = provider.heroku app = tftest-app-iycoxl7pzx config_vars.# = 1 config_vars.0 = HOSTEDGRAPHITE_APIKEY name = hostedgraphite-flat-54405 plan = hostedgraphite:free provider_id = e790bfc4 Dependencies: heroku_app.app heroku_app.app: ID = tftest-app-iycoxl7pzx provider = provider.heroku acm = false all_config_vars.% = 1 all_config_vars.HOSTEDGRAPHITE_APIKEY = 25da0edc-c571-4664-b578-cae2b6b040e7 config_vars.# = 0 git_url = https://git.heroku.com/tftest-app-iycoxl7pzx.git heroku_hostname = tftest-app-iycoxl7pzx.herokuapp.com internal_routing = false name = tftest-app-iycoxl7pzx region = us stack = heroku-16 web_url = https://tftest-app-iycoxl7pzx.herokuapp.com/ heroku_app.app2: ID = tftest-app-iycoxl7pzx-2 provider = provider.heroku acm = false all_config_vars.% = 1 all_config_vars.HOSTEDGRAPHITE_APIKEY = 25da0edc-c571-4664-b578-cae2b6b040e7 config_vars.# = 1 config_vars.0.% = 1 config_vars.0.HOSTEDGRAPHITE_APIKEY = 25da0edc-c571-4664-b578-cae2b6b040e7 git_url = https://git.heroku.com/tftest-app-iycoxl7pzx-2.git heroku_hostname = tftest-app-iycoxl7pzx-2.herokuapp.com internal_routing = false name = tftest-app-iycoxl7pzx-2 region = us stack = heroku-16 web_url = https://tftest-app-iycoxl7pzx-2.herokuapp.com/ Dependencies: data.heroku_app_config_vars.app_config_vars FAIL FAIL github.com/terraform-providers/terraform-provider-heroku/heroku 11.348s Error: Tests failed.

For the life of me, I can't understand why the subsequent plan after running apply comes back trying to create data.heroku_app_config_vars.app_config_vars, where app2 managed to pull off the right config values off the new data resource.

I don't have a direct answer for you, but may I suggest running the tests with TF_LOG=DEBUG or TF_LOG=TRACE?

(https://github.com/terraform-providers/terraform-provider-heroku/blob/master/TESTING.md)

@shirdoo I suspect it's because config_vars are marked as Computed: true?

We've also ran into situations where the API returns a success, but async operations haven't completed behind the scenes. Unsure if that might apply to application config vars.

finally narrowed it down, the issue is using depends_on. For some reason, when that attribute is set, the data source is not saving any remote state, so all the apply's in the world still have the data source coming up as a new resource to create.

shirdoo · 2018-08-14T23:25:40Z

@bernerdschaefer, quick review when you get a chance? I'm not happy with the changes outstanding after running apply, any ideas why I'm getting that would be great

shirdoo · 2018-08-16T00:17:02Z

heroku/data_source_heroku_app_config_vars_test.go

+
+data "heroku_app_config_vars" "app_config_vars" {
+  app = "${heroku_app.app.name}"
+  depends = ["${heroku_addon.hostedgraphite.app}"]


I'm not too happy about this, there's a bug somewhere in the terraform provider that breaks a data source when used in conjunction with depends_on.

Instead I'm sticking a dummy attribute to stick inferred dependencies on other terraform resources, so the data resource waits until the addon is added, before trying to slap configs onto downstream resources (ie: app2 doing a lookup of data.heroku_app_config_vars.app_config_vars before the hosted graphite addon getting attached. I should see if there's an outstanding issue in terraform

I guess I'm looking for advice on whether we want to document this depends attribute to reflect this

voila, here's the issue I hit: hashicorp/terraform#11806

@talbright @bernerdschaefer @joestump for another quick reivew, thanks in advance!

davidji99 · 2018-08-16T17:23:00Z

website/docs/d/heroku_app_config_vars.html.markdown

+## Example Usage
+
+```hcl
+# Look up a Heroku Private Space's peering info. 


I guess you copied this example hcl from somewhere else :P

davidji99 · 2018-08-16T17:23:11Z

website/docs/d/heroku_app_config_vars.html.markdown

+  app   = "${heroku_app.foo.name}"
+}
+
+# Initiate a VPC peering connection request.


davidji99 · 2018-08-16T17:30:20Z

heroku/data_source_heroku_app_config_vars.go

+				Type:     schema.TypeMap,
+				Computed: true,
+			},
+			"depends": {


I would consider changing depends to maybe wait_for_resources or wait_for. depends seems a bit too close to depends_on and could confuse users.

Additionally I think if we were to move forward with this attribute, I suggest changing the Type to schema.TypeSet so that the ordering doesn't matter.

…vider-heroku into config_var_data

degaydorus · 2018-08-17T18:03:44Z

heroku/data_source_heroku_app_config_vars.go

+
+func dataSourceHerokuAppConfigVarsRead(d *schema.ResourceData, m interface{}) error {
+	client := m.(*heroku.Service)
+


If m doesn't hold a value of type *heroku.Service it will panic.
you can check the type to make sure a panic doesn't happen if the type doesn't convert.
client, ok := m.(*heroku.Service)
and proceed if okay is true.

Its a valid point, that being said we aren't checking the type conversion anywhere else in the code base, so I wouldn't necessarily ding this PR for that.

degaydorus · 2018-08-17T18:18:37Z

heroku/data_source_heroku_app_config_vars_test.go

+
+func testAccCheckHerokuAppConfigVar(appName string) string {
+	return fmt.Sprintf(`
+resource "heroku_app" "app" {


you can turn on gofmt in your ide to fix the formatting indentation

format passes fine for me:

gofmt -l heroku/data_source_heroku_app_config_vars_test.go

yeah, I think its the backtick making it look weird, but seems gofmt happy

shirdoo · 2018-08-17T20:08:30Z

@davidji99 @bernerdschaefer can one of you take another pass at this? thanks in advance!

shirdoo · 2018-08-20T21:24:44Z

gentle poke @davidji99 @bernerdschaefer

davidji99 · 2018-08-21T14:29:20Z

👀

davidji99 · 2018-08-23T17:33:02Z

heroku/data_source_heroku_app_config_vars.go

+func dataSourceHerokuAppConfigVarsRead(d *schema.ResourceData, m interface{}) error {
+	client := m.(*heroku.Service)
+
+	appName := d.Get("app").(string)


https://github.com/terraform-providers/terraform-provider-heroku/blob/master/heroku/helper.go#L12 could use this helper here.

davidji99 · 2018-08-23T17:33:15Z

heroku/data_source_heroku_app_config_vars.go

+				Type:     schema.TypeMap,
+				Computed: true,
+			},
+			"wait_for_resources": {


I had a chat with one of the hashicorp guys on this PR. He offered an suggestion:

or perhaps it could work a bit like consul_keys where you have to identify which variable names you are looking for, and then the data source could wait until those are ready before returning

Maybe we could do this? a wait_for_keys or wait_for_vars where one specifies the keys you're looking for and have a max timeout of say 5min? If this attribute is not set, then just refresh as normal. This might give you and anyone else more control in making sure the data resource has what you need? What do you think Sharath?

talbright · 2019-03-27T13:52:13Z

closing this due to inactivity: feel free to re-open if you wish to pursue this further.

sbabu-salesforce added 2 commits August 14, 2018 13:59

add heroku_app_config_vars data resource

1fd31dc

remove unnecessary field on config_vars data resource

86d6b8b

shirdoo mentioned this pull request Aug 14, 2018

heroku_config resource #47

Closed

shirdoo commented Aug 14, 2018

View reviewed changes

sbabu-salesforce added 2 commits August 14, 2018 14:16

add docs

24bb822

fix doc example

aa96d42

dont use depends_on attribute

44203d5

shirdoo commented Aug 16, 2018

View reviewed changes

add docs to depends attribute

f3ef6e0

davidji99 reviewed Aug 16, 2018

View reviewed changes

sbabu-salesforce added 3 commits August 16, 2018 14:14

address minor review comments

dfcc392

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

d7f9d87

…vider-heroku into config_var_data

merge with master

2b9ccc1

degaydorus reviewed Aug 17, 2018

View reviewed changes

davidji99 reviewed Aug 23, 2018

View reviewed changes

talbright closed this Mar 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config var data #109

Config var data #109

shirdoo commented Aug 14, 2018

talbright commented Aug 14, 2018

shirdoo Aug 14, 2018

talbright Aug 15, 2018 •

edited

Loading

joestump Aug 15, 2018

joestump Aug 15, 2018

shirdoo Aug 16, 2018

shirdoo commented Aug 14, 2018

shirdoo Aug 16, 2018

shirdoo Aug 16, 2018

shirdoo Aug 16, 2018

shirdoo Aug 16, 2018

davidji99 Aug 16, 2018

davidji99 Aug 16, 2018

davidji99 Aug 16, 2018

degaydorus Aug 17, 2018

talbright Aug 17, 2018

degaydorus Aug 17, 2018

talbright Aug 17, 2018

shirdoo Aug 17, 2018

shirdoo commented Aug 17, 2018

shirdoo commented Aug 20, 2018

davidji99 commented Aug 21, 2018

davidji99 Aug 23, 2018

davidji99 Aug 23, 2018 •

edited

Loading

talbright commented Mar 27, 2019


		func dataSourceHerokuAppConfigVarsRead(d *schema.ResourceData, m interface{}) error {
		client := m.(*heroku.Service)

Config var data #109

Config var data #109

Conversation

shirdoo commented Aug 14, 2018

talbright commented Aug 14, 2018

Choose a reason for hiding this comment

talbright Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shirdoo commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shirdoo commented Aug 17, 2018

shirdoo commented Aug 20, 2018

davidji99 commented Aug 21, 2018

Choose a reason for hiding this comment

davidji99 Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

talbright commented Mar 27, 2019

talbright Aug 15, 2018 •

edited

Loading

davidji99 Aug 23, 2018 •

edited

Loading