Add Add-on attachment name

[brian-mcnamara]

No description provided.

[brian-mcnamara]

This was named based on the heroku cli --as parameter on addon:create
https://devcenter.heroku.com/articles/heroku-cli-commands#heroku-addons-create-service-plan

[bernerdschaefer]

Thanks @brian-mcnamara!

Could you also update https://github.com/terraform-providers/terraform-provider-heroku/blob/master/website/docs/r/addon.html.markdown#argument-reference and https://github.com/terraform-providers/terraform-provider-heroku/blob/master/CHANGELOG.md#222-unreleased?

[bernerdschaefer]

The updates look great, thanks @brian-mcnamara!

There's one test failing (from the previous version):

------- Stdout: -------
=== RUN   TestAccHerokuAddon_importBasic
--- FAIL: TestAccHerokuAddon_importBasic (9.61s)
    testing.go:569: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=1) {
         (string) (len=2) "as": (string) (len=9) "foobar_as"
        }
        
FAIL

It looks like we probably need to include attachment_name in the list of ignored attributes in the import test, since it's a create-only field and won't be read into state during import: https://github.com/terraform-providers/terraform-provider-heroku/blob/master/heroku/import_heroku_addon_test.go#L26

Once that's passing this should be all set! 👍

[davidji99]

Since I can't do an inline comment, you also need to set the attachment name in the Read function: https://github.com/terraform-providers/terraform-provider-heroku/pull/241/files#diff-1a1b7e50595b433f6c7e741170d655b8R184

[davidji99]

This PR is a duplicate of the one I was working on #227. The implementation between the two is mostly the same. I listed a few questions I had when I was working on it so I'd like to know if you have tested/researched the following questions:

1. What happens when someone has a heroku_addon_attachment resource that specifies a different name? Could this cause an infinite dirty plan?
2. Any issues when running a new provider version on an existing tf configuration code base?

I am circling back to #227 since my team now needs it so if the comments I added to this PR require additional modifications, I would like to finish off my PR and merge it in.

FYI @bernerdschaefer

[bernerdschaefer]

Since I can't do an inline comment, you also need to set the attachment name in the Read function: https://github.com/terraform-providers/terraform-provider-heroku/pull/241/files#diff-1a1b7e50595b433f6c7e741170d655b8R184

Note: the addon resource does not return the attachment name, so it can't be read back.

1. What happens when someone has a heroku_addon_attachment resource that specifies a different name? Could this cause an infinite dirty plan?

The attachment name set on creation cannot be updated via the addon APIs. I think @brian-mcnamara's changes here are the correct way to model the interaction -- ForceNew, set the attribute during create, and don't set it during read. That said... it would be confusing and weird if you did change the attachment name via an attachment resource, but then couldn't change or remove the addon_attachment attribute on the addon without terraform thinking it needed to be destroyed and recreated.

[brian-mcnamara]

Since I can't do an inline comment, you also need to set the attachment name in the Read function: https://github.com/terraform-providers/terraform-provider-heroku/pull/241/files#diff-1a1b7e50595b433f6c7e741170d655b8R184

Note: the addon resource does not return the attachment name, so it can't be read back.

1. What happens when someone has a heroku_addon_attachment resource that specifies a different name? Could this cause an infinite dirty plan?

The attachment name set on creation cannot be updated via the addon APIs. I think @brian-mcnamara's changes here are the correct way to model the interaction -- ForceNew, set the attribute during create, and don't set it during read. That said... it would be confusing and weird if you did change the attachment name via an attachment resource, but then couldn't change or remove the addon_attachment attribute on the addon without terraform thinking it needed to be destroyed and recreated.

Thanks for filling in, you nailed it. The issue about a recreate happening for a attachment_name change may be able to be fixed. Let me play around with the heroku API tomorrow and see if I can't change it without destroying the created add-on. My current thought is it would require a large refactor to the add-on state (which may not work for forwards compatibility). Ill post back.

Thanks for all the support on this one everyone!

[brian-mcnamara]

Testing some things:

Given an add-on id, you can query the heroku api to list the add-on attachment that were also created. You can not delete the only attachment associated to an add-on but can create a new one, followed by deleting the old one. If we want to make sure that users can change the attachment_name without destroying and recreating the add-on we would need to:

• Update state so an add-on creates an add-on and add-on attachment in terraform state
• During change, create new attachment and delete the old attachment while the add-on remains untouched

This may be possible, but my initial concerns are:

• Forwards compatibility (how do we upgrade state for prior versions).
• Need to make sure a user can not add an add-on attachment that mirrors the attachment_name defined in the add-on (if there is one).
• Multiple API calls needed during create (create add-on, fetch attachment).

Thoughts?

[davidji99]

@bernerdschaefer Yeah it's not possible but you could do a roundabout way via https://devcenter.heroku.com/articles/platform-api-reference#add-on-attachment-list or better yet, https://devcenter.heroku.com/articles/platform-api-reference#add-on-attachment-list-by-add-on. That's how I implemented the READ in my PR.

[brian-mcnamara]

Oh geez, @davidji99 I didn't notice your PR until now 😮, do you have time to work on completing your PR? Happy to help work on your branch (since it looks like you have a lot of things I re-did)

[davidji99]

Oh geez, @davidji99 I didn't notice your PR until now 😮, do you have time to work on completing your PR? Happy to help work on your branch (since it looks like you have a lot of things I re-did)

Yeah, let me circle back to my PR.

[ypaq]

What is missing here to move this forward?

[DanielWright]

I'll be the guy who raises this from the dead to ask: any timeline on getting this merged and released? 😇

[mars]

I've read through all these interlinked issues & PRs, and do not see how this can be merged, because:

• the heroku_addon attachment name cannot be read nor confidently inferred after creation
• making heroku_addon attachment name ForceNew could cause data loss through add-on recreation.

Create could work, but we just don't have enough information to correctly manage the lifecycle of the default Add-on attachment as part of the heroku_addon resource.

Could the solution be to use heroku_addon_attachment with its name attribute, anytime you need a named attachment?

[mars]

Workaround 🧰

When an add-on attachment name is required for your use-case, create a new heroku_app (just to host the add-on) and on that app a heroku_addon.

Then, create heroku_addon_attachment with the subject addon_id, target app_id, and a name as desired.