Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix attachment IDs #39

Closed
wants to merge 1 commit into from
Closed

Fix attachment IDs #39

wants to merge 1 commit into from

Conversation

cloudartisan
Copy link

@cloudartisan cloudartisan commented Jan 31, 2018

Fixes #40

The attachment resource is saving add-on name as add-on ID. This results in a change on every terraform plan because add-on name != add-on ID.

Terraform will perform the following actions:

heroku_addon_attachment.sumologic (new resource required)
       id:       "5382910f-92d4-4952-bc39-a92de2f87a72" => <computed> (forces new resource)
       addon_id: "sumologic-cubic-53500" => "1c7aa1ef-a6fc-47a4-abbb-b23512fc15a7" (forces new resource)
       app_id:   "sfdc-cb-rt-stg-ore" => "sfdc-cb-rt-stg-ore"
       name:     "SUMOLOGIC" => <computed>

Plan: 1 to add, 0 to change, 1 to destroy.

Note the add-on name sumologic-cubic-53500 has been saved in the state file as the add-on ID for the heroku_addon_attachment resource. It should have been saved as 1c7aa1ef-a6fc-47a4-abbb-b23512fc15a7 in the example above, not sumologic-cubic-53500.

Copy link
Contributor

@justincampbell justincampbell left a comment

Choose a reason for hiding this comment

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

The acceptance tests fail with this change. I ran:

$ TESTARGS="-run AddonAttachment" make testacc

I noticed the acceptance tests are also slightly incorrect. We create an app and an addon (attached to the app), and then create an addon_attachment to attach the addon to the app again. I think a better test would be to have 2 apps, 1 addon, and then attach the addon to the 2nd app.

@cloudartisan
Copy link
Author

@justincampbell thanks for the feedback. Yesterday was the first time in my life I touched a line of Go and couldn't find any good instructions on contributing. I did run make test and didn't get any errors, so assumed all was well with the existing tests. I can take a look, maybe later today, when I have more time to try to understand the Go code of the test suite.

@joestump
Copy link
Contributor

I suspect it's failing on tests where app_id is expected to be appName: https://github.com/terraform-providers/terraform-provider-heroku/blob/master/heroku/resource_heroku_addon_attachment_test.go#L21

@joestump
Copy link
Contributor

I think the fix is to remove appName from the those test checks and use TestCheckResourceAttrSet instead of TestCheckResourceAttr.

@davidji99 davidji99 mentioned this pull request Jun 8, 2018
@davidji99
Copy link
Collaborator

@cloudartisan I am closing this PR. I merged your change in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants