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

Starting point for adding Organization app support for Heroku Provider #389

Merged
merged 4 commits into from
Oct 14, 2014
Merged

Starting point for adding Organization app support for Heroku Provider #389

merged 4 commits into from
Oct 14, 2014

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Oct 10, 2014

This is a hack-ish first pass at adding Organization app support (1,2) to the Heroku Provider.

This PR is meant as a starting point for discussion because frankly it's ugly :)

With this PR, you can create an app under and Organization by adding the org name to the resource:

resource "heroku_app" "web" {
  name         = "catsby-test-app"
  region       = "us"
  organization = "catsby"
}
    return &schema.Resource{
-       Create: resourceHerokuAppCreate,
+       Create: switchHerokuAppCreate,

The intent here is to conditionally return the right method to create the app, depending on if the organization field was found in the schema. The name is not the best :/

For the actual conditional logic:

+   if len(isOrg.(string)) != 0 {

This seems horrible, but a previous check for nil was wrong because the organization field seems to get the default string value "" (I think).

+func resourceHerokuOrgAppCreate(d *schema.ResourceData, meta interface{}) error {

I copy-pasted 99% of this method from resourceHerokuAppCreate, only adding/changing org specific things, so there's ugly duplication. Still being a Go novice, I wasn't sure how to separate the similar logic and change the methods to be specific to their cases.

I wasn't sure how far to go with the Organization support, as there are other fields like locked, joined, etc. Maybe the organization should be a map in the configuration, like so?

resource "heroku_app" "web" {
  name         = "catsby-test-app"
  region       = "us"
  organization {
    name = "catsby"
    locked = true
  }
}

I would really like feedback and guidance on DRY/cleanup suggestions here, as well as guidance on how to write a test for this. I've verified it works, but I'm not sure where to start on the testing :/

@catsby
Copy link
Contributor Author

catsby commented Oct 10, 2014

Rebased on master. Not sure why tests are failing on Travis, they pass locally 🐼

@mitchellh
Copy link
Contributor

You can use if _, ok := d.GetOk("organization"); ok to check if something is set. Get always returns the "zero" value for a type, as you discovered, but GetOk will tell you if its set.


"organization": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing that changing this would require a new app to be created? i.e. you can't "update" an app to be part of an organization? If so, you need to set ForceNew: true here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, the Platform API seems limited here; you can't use it to move an app to an organization. There's App Transfering, but that requires a human on the other side.

Added ForceNew: true in 5fe2593.

@mitchellh
Copy link
Contributor

This looks great! Given the API of the Heroku lib, I don't think it is very possible to DRY up the create method unfortunately. With the given fixes, I'd be happy to merge this in.

@catsby
Copy link
Contributor Author

catsby commented Oct 12, 2014

@mitchellh I've made updates based on your feedback, thanks!

I've also pushed another commit on my fork from a separate branch, based on this one, that changes organization from a simple String to a map of fields, supporting the locked and personal fields. Thoughts on that approach? I'm fine leaving it as is here and just supporting the name as well.

@mitchellh
Copy link
Contributor

Woot, made it in the 11th hour in 0.3. :)

mitchellh added a commit that referenced this pull request Oct 14, 2014
Starting point for adding Organization app support for Heroku Provider
@mitchellh mitchellh merged commit e4a2cb2 into hashicorp:master Oct 14, 2014
@catsby
Copy link
Contributor Author

catsby commented Oct 14, 2014

Yay 🤘

I'll send another PR for the organization string -> map idea

@chids
Copy link
Contributor

chids commented Oct 14, 2014

💖

@ghost ghost locked and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants