WIP: heroku_app datasource #1

sheax0r · 2018-02-06T05:16:22Z

This pull request is intended to enable heroku_app data objects in terraform, like this:

data "heroku_app" "my_app" {
  name = "my-app"
}

This object exposes the following attributes:

Xe · 2018-02-06T05:40:51Z

heroku/data_source_heroku_app.go

+func dataSourceHerokuApp() *schema.Resource {
+	return &schema.Resource{
+		Read: dataSourceHerokuAppRead,
+		Schema: map[string]*schema.Schema{


Please be sure these are documented somewhere.

I don't have experience with data sources, but I wonder if it would be useful to re-use the existing app resource schema here? Something like Schema: resourceHerokuApp().Schema?

For that matter, could you use resourceHerokuAppRead as the reader here to also re-use all of that logic?

@bernerdschaefer I'll try to DRY this up later on, but I'm not sure how much I can do regarding schema reuse. The schema actually is subtly different - for instance, elements tend to be "required" in the resource schema, but "computed" in the datasource schema.

I'm using aws resources and datasources for reference (this is also my first experience writing datasources, or any terraform go code).

Ah, interesting. The repetition in read bothers me more than in the schema since there's rather a lot going on, but neither bothers me so badly that we couldn't ship this as a first pass.

@bernerdschaefer I've refactored a bunch of the logic around retrieving data for an app, so that I can reuse stuff from resource_heroku_app inside of datasource_heroku_app.

@Xe I've added docs on datasource_heroku_app, using the existing heroku_app resource docs plus some aws datasource docs as a template.

It also now retrieves a bunch more stuff than the first pass did - buildpacks, config vars, organization info.

I still need to manually test organization and space functionality (so far I've only been testing with a common runtime app), and I need to write some tests.

jmervine · 2018-02-06T07:02:05Z

We should look at adding support for drains while we’re in here. If it isn’t already here somewhere.

sheax0r · 2018-02-06T07:09:28Z

So far all this does is a subset of heroku_app. We should make our data objects correspond to the resource objects.

A drain is its own resource (see here); by all means add it. I feel like we should just try to add as much stuff as we can do (reasonably well) within the hackathon timebox.

bernerdschaefer

I can see some great uses for these data sources!

bernerdschaefer · 2018-02-06T17:07:01Z

heroku/data_source_heroku_app.go

+func dataSourceHerokuApp() *schema.Resource {
+	return &schema.Resource{
+		Read: dataSourceHerokuAppRead,
+		Schema: map[string]*schema.Schema{


I don't have experience with data sources, but I wonder if it would be useful to re-use the existing app resource schema here? Something like Schema: resourceHerokuApp().Schema?

For that matter, could you use resourceHerokuAppRead as the reader here to also re-use all of that logic?

bernerdschaefer · 2018-02-06T17:07:47Z

heroku/data_source_heroku_app.go

+		return err
+	}
+
+	d.SetId(time.Now().UTC().String())


Interesting. Did you pick this up from how other data sources work?

@bernerdschaefer I did - I got it from an AWS data source. acm_certificate, I think.

Consider d.SetId("hk-"+time.Now().UTC().String())?

@jmervine are we concerned that there is some sort of potential negative effect from doing it this way, like collision of IDs? I literally copied this from the code for aws_acm_certificate, and I assumed (perhaps incorrectly) that they know what they're doing.

Nah, I was just thinking about AWS does it, e.g. instances have i-<id>. But I defer to you. It was just a thought.

jmervine · 2018-02-06T21:17:55Z

heroku/data_source_heroku_app.go

+			{"name": app.Organization.Name},
+		})
+	} else {
+		d.Set("organization", "")


Should this be an empty map, or perhaps nil when not present, or does an empty string work and make sense here?

@jmervine so far in my testing - there are problems with using nil. Basically, I've got this terraform script:

data "heroku_app" "parent" { name = "${var.parent_app}" } resource "heroku_app" "child" { name = "${var.child_app}" region = "us-east" config_vars { ORGANIZATION = "${data.heroku_app.parent.organization}" REGION = "${data.heroku_app.parent.region}" SPACE = "${data.heroku_app.parent.space}" STACK = "${data.heroku_app.parent.stack}" GIT_URL = "${data.heroku_app.parent.git_url}" WEB_URL = "${data.heroku_app.parent.web_url}" } }

I finding that if organization (or any other attribute) is nil, the terraform script will fail to apply, with errors like this:

* heroku_app.child: Resource 'data.heroku_app.parent' does not have attribute 'space' for variable 'data.heroku_app.parent.space'

So the question here is: Should the script actually fail if the app doesn't have an organization? My belief is "no". I think it's better to return an empty string.

I see three possiblities:

I'm doing something wrong and it is possible to have a nil value for an attribute but still have attribute work if referenced (so far, though, I don't think I'm wrong about this).

You actually shouldn't try to reference an attribute that is nil (this seems silly though - the script becomes very brittle if I want to apply the same recipe to different resources where some of them have an organization but others don't).

What I'm doing in the script is correct, as is the way I'm continuing to expose "nil" attributes as empty strings.

BTW it looks like "how to represent nil attributes in terraform" is
a problem others have encountered: hashicorp/terraform#5471 (comment)

I tend to agree. Origination is optional and should be treated as such. The terraform pattern is typically to omit the config, if it's not required and not present. AFAIK, if most of the AWS resources, there is no way of saying "nil so fall back on default", if you set it to something, then it errors or applies what you set it to, even if that's an empty string.

That doesn't innately allow for reuse, unfortunately.

So if I'm understanding you correctly, if you don't handle the nil, it errors. How does the heroku lib handle the empty string? If the heroku lib handles an empty string, I'd go that route... I'd also be surprised if it does.

My other suggestion was to set it to an empty map, e.g.

d.Set("organization", []map[string]interface{}{})

Might that be better?

Err wait. What are you agreeing with? There were three possibilities, you can't agree with all of them. I'm not sure what you mean by "origination is optional and should be treated as such".

Based on the rest of your comment, I think you're agreeing with option 2.

@jmervine it looks like, for consistency, option 2 is appropriate. Here's what happens if you try to reference "organization" for a heroku_app resource that doesn't have one:

* heroku_app.child: Resource 'heroku_app.managed' does not have attribute 'organization' for variable 'heroku_app.managed.organization'

... This is based on this terraform script:

data "heroku_app" "parent" { name = "${var.parent_app}" } resource "heroku_app" "managed" { name = "shea-managed-1" region = "us-east" } resource "heroku_app" "child" { name = "${var.child_app}" region = "us-east" config_vars { ORGANIZATION = "${data.heroku_app.parent.organization}" REGION = "${data.heroku_app.parent.region}" SPACE = "${data.heroku_app.parent.space}" STACK = "${data.heroku_app.parent.stack}" GIT_URL = "${data.heroku_app.parent.git_url}" WEB_URL = "${data.heroku_app.parent.web_url}" MANAGED_ORGANIZATION = "${heroku_app.managed.organization}" } }

So, I'm going to simply fail if users try to reference nil attributes. I think this is a shortcoming of terraform, but it's not one that is really possible to work around in any good way at this point.

Is there even possible to pass a nil value via Terraform? I thought it was omit the config or set it to a string or map. Are empty maps (or blocks in terraform DSL) even possible?

I was agreeing with 'Should the script actually fail if the app doesn't have an organization? My belief is "no".'

Xe · 2018-02-08T04:28:06Z

website/docs/d/app.html.markdown

+sidebar_current: "docs-heroku-datasource-app-x"
+description: |-
+  Get information on a Heroku App.
+---


👍 👍 👍

Xe · 2018-02-09T01:55:46Z

website/docs/d/app.html.markdown

@@ -30,8 +30,8 @@ The following arguments are supported:

 The following attributes are exported:

-* `name` - (Required) The name of the application. In Heroku, this is also the
-   unique ID.
+* `name` - (Required) The nam of the application. In Heroku, this is also the


jmervine · 2018-02-09T07:22:50Z

website/docs/r/app.html.markdown

@@ -41,10 +41,6 @@ The following arguments are supported:
 * `buildpacks` - (Optional) Buildpack names or URLs for the application.
     Buildpacks configured externally won't be altered if this is not present.
 * `config_vars` - (Optional) Configuration variables for the application.
-     The config variables in this map are not the final set of configuration


@sheax0r Are you removing this comment because the functionality has changed?

@jmervine I'm removing it because it's incorrect; the removed text was copied from the heroku_app resource, where it applies - but the datasource is slightly different.

The data object exposes all config vars in its config_vars attribute.

The resource exposes only certain config vars in its config_vars attribute - ie, ones that the user has decided to care about.

The datasource and the resource are necessarily different in this respect.

Oh wait @jmervine I see what you're saying now! I removed this text from the resource, not the datasource! :D.

My bad. I will put it back.

Xe · 2018-02-09T19:12:15Z

website/docs/d/app.html.markdown

@@ -30,7 +30,7 @@ The following arguments are supported:

 The following attributes are exported:

-* `name` - (Required) The nam of the application. In Heroku, this is also the
+* `name` - (Required) The name of the application. In Heroku, this is also the


sheax0r force-pushed the heroku-app-datasource branch from fb77166 to 1745ddf Compare February 6, 2018 05:33

Xe reviewed Feb 6, 2018

View reviewed changes

bernerdschaefer approved these changes Feb 6, 2018

View reviewed changes

jmervine reviewed Feb 6, 2018

View reviewed changes

Xe reviewed Feb 8, 2018

View reviewed changes

website/docs/d/app.html.markdown

sidebar_current: "docs-heroku-datasource-app-x"

description: |-

Get information on a Heroku App.

---

Copy link

Xe Feb 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍

Xe reviewed Feb 9, 2018

View reviewed changes

jmervine reviewed Feb 9, 2018

View reviewed changes

jmervine approved these changes Feb 9, 2018

View reviewed changes

Xe reviewed Feb 9, 2018

View reviewed changes

Initial implementation of heroku_app data source and tests.

735fc12

sheax0r force-pushed the heroku-app-datasource branch from cddd3da to 735fc12 Compare February 12, 2018 01:14

Add heroku_app datasource to sidebar

cced324

sheax0r merged commit cced324 into master Apr 30, 2019

sheax0r pushed a commit that referenced this pull request Apr 30, 2019

provider_test tweak #1 [issues/95-support-for-netrc-authent]

1503fe3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: heroku_app datasource #1

WIP: heroku_app datasource #1

sheax0r commented Feb 6, 2018 •

edited

Loading

Xe Feb 6, 2018

bernerdschaefer Feb 6, 2018

sheax0r Feb 6, 2018

bernerdschaefer Feb 6, 2018

sheax0r Feb 8, 2018

jmervine commented Feb 6, 2018

sheax0r commented Feb 6, 2018 •

edited

Loading

bernerdschaefer left a comment

bernerdschaefer Feb 6, 2018

bernerdschaefer Feb 6, 2018

sheax0r Feb 6, 2018

jmervine Feb 6, 2018 •

edited

Loading

sheax0r Feb 8, 2018

jmervine Feb 8, 2018

jmervine Feb 6, 2018 •

edited

Loading

sheax0r Feb 6, 2018

jmervine Feb 6, 2018 •

edited

Loading

jmervine Feb 6, 2018 •

edited

Loading

sheax0r Feb 6, 2018

sheax0r Feb 6, 2018

sheax0r Feb 6, 2018

jmervine Feb 6, 2018

Xe Feb 8, 2018

Xe Feb 9, 2018

jmervine Feb 9, 2018

sheax0r Feb 9, 2018

sheax0r Feb 9, 2018

Xe Feb 9, 2018

WIP: heroku_app datasource #1

WIP: heroku_app datasource #1

Conversation

sheax0r commented Feb 6, 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

Choose a reason for hiding this comment

jmervine commented Feb 6, 2018

sheax0r commented Feb 6, 2018 • edited Loading

bernerdschaefer left a 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

jmervine Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmervine Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmervine Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

jmervine Feb 6, 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

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

sheax0r commented Feb 6, 2018 •

edited

Loading

sheax0r commented Feb 6, 2018 •

edited

Loading

jmervine Feb 6, 2018 •

edited

Loading

jmervine Feb 6, 2018 •

edited

Loading

jmervine Feb 6, 2018 •

edited

Loading

jmervine Feb 6, 2018 •

edited

Loading