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

providers/heroku: import heroku_pipeline_coupling resource #14495

Merged
merged 1 commit into from
May 17, 2017

Conversation

cmorent
Copy link
Contributor

@cmorent cmorent commented May 15, 2017

Adds support for importing pipelines with the heroku_pipeline_coupling resource.

Resolves #14491

@cmorent cmorent force-pushed the cm-import-heroku-pipeline-coupling branch from db7800d to 1b2c2d1 Compare May 15, 2017 13:00
@cmorent cmorent changed the title providers/heroku: import heroku_pipeline_coupling resource [WIP] providers/heroku: import heroku_pipeline_coupling resource May 15, 2017
@catsby
Copy link
Contributor

catsby commented May 15, 2017

Hey @cmorent is this still WIP? And can we simply use schema.ImportStatePassthrough instead of a custom import method here?

@catsby catsby added provider/heroku enhancement waiting-response An issue/pull request is waiting for a response from the community labels May 15, 2017
@cmorent
Copy link
Contributor Author

cmorent commented May 15, 2017

Hey @catsby, yep, this is still WIP. Thanks, I did not know about this ImportStatePassthrough, I'll try to use it here and let you know if it works as expected !

@cmorent cmorent force-pushed the cm-import-heroku-pipeline-coupling branch from 1b2c2d1 to 33118e6 Compare May 15, 2017 15:15
@cmorent
Copy link
Contributor Author

cmorent commented May 15, 2017

Hey again @catsby, I am testing using the schema.ImportStatePassthrough and it works but it only imports the id and the stage (pipeline and app are missing). Do you know if there's any workaround or if I will have to use a custom import method? Thanks in advance !

@cmorent cmorent force-pushed the cm-import-heroku-pipeline-coupling branch from 33118e6 to 9f80bce Compare May 16, 2017 07:49
@catsby
Copy link
Contributor

catsby commented May 16, 2017

Hey @cmorent so I dug in here a bit and found some things:

  • the d.Set for app and pipelineare actually failing (d.Set returns an error, but for simple strings it's rarely checked). p.Pipeline and p.App here are actually structs.
  • the App returned in p.App only contains the app ID, but this resource tracks on the app name

I tinkered this morning and came up with this patch, which I think makes everything nice. It also adds a computed value app_id to the schema:

diff --git a/builtin/providers/heroku/resource_heroku_pipeline_coupling.go b/builtin/providers/heroku/resource_heroku_pipeline_coupling.go
index 4d513c363..cc0668af4 100644
--- a/builtin/providers/heroku/resource_heroku_pipeline_coupling.go
+++ b/builtin/providers/heroku/resource_heroku_pipeline_coupling.go
@@ -20,6 +20,10 @@ func resourceHerokuPipelineCoupling() *schema.Resource {
 		},
 
 		Schema: map[string]*schema.Schema{
+			"app_id": {
+				Type:     schema.TypeString,
+				Computed: true,
+			},
 			"app": {
 				Type:     schema.TypeString,
 				Required: true,
@@ -85,9 +89,16 @@ func resourceHerokuPipelineCouplingRead(d *schema.ResourceData, meta interface{}
 		return fmt.Errorf("Error retrieving pipeline: %s", err)
 	}
 
-	d.Set("app", p.App)
-	d.Set("pipeline", p.Pipeline)
+	// grab App info
+	app, err := client.AppInfo(context.TODO(), p.App.ID)
+	if err != nil {
+		log.Printf("[WARN] Error looking up addional App info for pipeline coupling (%s): %s", d.Id(), err)
+	}
+
+	d.Set("app_id", p.App.ID)
+	d.Set("app", app.Name)
 	d.Set("stage", p.Stage)
+	d.Set("pipeline", p.Pipeline.ID)
 
 	return nil
 }

With that, schema.ImportStatePassthrough works as expected, and the tests still pass:

TF_ACC=1 go test ./builtin/providers/heroku -v -run=TestAccHerokuPipeline -timeout 120m
=== RUN   TestAccHerokuPipelineCoupling_importBasic
--- PASS: TestAccHerokuPipelineCoupling_importBasic (6.00s)
=== RUN   TestAccHerokuPipeline_importBasic
--- PASS: TestAccHerokuPipeline_importBasic (1.68s)
=== RUN   TestAccHerokuPipelineCoupling_Basic
--- PASS: TestAccHerokuPipelineCoupling_Basic (5.93s)
=== RUN   TestAccHerokuPipeline_Basic
--- PASS: TestAccHerokuPipeline_Basic (2.27s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/heroku 15.887s

Feel free to take the patch above and add it here 😄

@catsby
Copy link
Contributor

catsby commented May 16, 2017

ack, of course I just noticed that we should only set the app if the API call to read it succeeds, so I'd put the d.Set("app"..) in an else block of the err check

@cmorent cmorent force-pushed the cm-import-heroku-pipeline-coupling branch from 9f80bce to f90e26e Compare May 17, 2017 14:04
@cmorent
Copy link
Contributor Author

cmorent commented May 17, 2017

Hey @catsby ! Thanks a lot for your inputs and for the time you've spent! It works perfectly with your patch and the tests for the import feature are passing too:

$ TF_ACC=1 go test ./builtin/providers/heroku -v -run=TestAccHerokuPipelineCoupling -timeout 120m                                            
=== RUN   TestAccHerokuPipelineCoupling_importBasic
--- PASS: TestAccHerokuPipelineCoupling_importBasic (9.40s)
=== RUN   TestAccHerokuPipelineCoupling_Basic
--- PASS: TestAccHerokuPipelineCoupling_Basic (8.24s)

PASS
ok  	github.com/cmorent/terraform/builtin/providers/heroku	17.673s

Also, if I try to import a heroku_pipeline_coupling resource now:

$ terraform import heroku_pipeline_coupling.foobar <pipeline_coupling_id>
heroku_pipeline_coupling.foobar: Importing from ID "<pipeline_coupling_id>"...
heroku_pipeline_coupling.foobar: Import complete!
  Imported heroku_pipeline_coupling (ID: <pipeline_coupling_id>)
heroku_pipeline_coupling.foobar: Refreshing state... (ID: <pipeline_coupling_id>)

Import success! The resources imported are shown above. These are
now in your Terraform state. Import does not currently generate
configuration, so you must do this next. If you do not create configuration
for the above resources, then the next `terraform plan` will mark
them for destruction.

$ terraform state show heroku_pipeline_coupling.foobar <pipeline_coupling_id>
id       = <heroku_pipeline_coupling_id>
app      = <heroku_pipeline_coupling_app>
app_id   = <heroku_app_id>
pipeline = <heroku_pipeline_coupling_pipeline>
stage    = <heroku_pipeline_coupling_stage>

Everything looks good to me now, thanks again!

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

👍

@catsby
Copy link
Contributor

catsby commented May 17, 2017

Looks good, thanks!

@catsby catsby merged commit 0c260d1 into hashicorp:master May 17, 2017
@cmorent cmorent changed the title [WIP] providers/heroku: import heroku_pipeline_coupling resource providers/heroku: import heroku_pipeline_coupling resource May 31, 2017
vanstee pushed a commit to vanstee/terraform that referenced this pull request Sep 28, 2017
@ghost
Copy link

ghost commented Apr 11, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement provider/heroku waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

heroku_pipeline_coupling does not support import
2 participants