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

Pubsub importable #392

Merged
merged 13 commits into from
Sep 11, 2017
Merged

Pubsub importable #392

merged 13 commits into from
Sep 11, 2017

Conversation

drzero42
Copy link
Contributor

@drzero42 drzero42 commented Sep 6, 2017

Fixes part of #386 by making pubsub_topic importable.

Making pubsub_subscription importable is more challenging, as resourcePubsubSubscriptionRead() doesn't actually do anything, meaning the created resource when importing, does not get any data filled in.

@rosbo rosbo self-requested a review September 6, 2017 20:36
@rosbo rosbo self-assigned this Sep 6, 2017
Copy link
Contributor

@rosbo rosbo left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, just a few changes before we merge it

}

name := d.Id()
id := fmt.Sprintf("projects/%s/topics/%s", project, name)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to overwrite the id here. The id should contain only the name. See the id format in the create method.

This is because d.Id() is being use in the read and delete method and the value is assumed to be name.

After moving the d.Set("name", ...) to the read method like suggested below, you can remove this method entirely and only use the passthrough handy function:

Importer: &schema.ResourceImporter{
			State: schema.ImportStatePassthrough,
		},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving d.Set("name", ...) to the read method makes sense, but if we move to using the passthrough import function the user would have to specify a much longer id during import - "projects/my-project/topics/my-topic", rather than just "my-topic", where the import method then interpolates the project name from the manifests.
I would personally prefer to just specify the name during import, but if the more experienced devs here prefer the longer form, I will change the PR to reflect this.
For now I have updated the PR to have the read method set the name instead of the import method doing it.
Let me know which way you want to go with this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am working on reusable id parser that will support both the self_link or just the name (inferring the project from the config) for a resource. This will be basically a generalization of what I did here: https://github.com/terraform-providers/terraform-provider-google/pull/378/files#diff-c969a5ee914c2e0cc17cc445f4b5ee22R169. No need for you to worry about this now. I will update the import method for all resources in one swoop. For now, just support the import by name only case.

The id format is currently only the name. It means that if you use a pass-through, then the import will require ONLY the name from the user. The current id format we use in Terraform for pubsub_topic is NOT "projects/%s/topics/%s". "projects/%s/topics/%s" is the relative part of the self_link of the resource. self_link != id (although they could be but this is not what is currently used for most resource).

Basically, you can delete your resourcePubsubTopicStateImporter method and use the pass-through and import will work by name:

Importer: &schema.ResourceImporter{
    State: schema.ImportStatePassthrough,
},

@drzero42
Copy link
Contributor Author

@rosbo I've removed the custom import method and switched it to using passthrough :)

@rosbo
Copy link
Contributor

rosbo commented Sep 11, 2017

I just realized that the API topic name format is not simply the name like the other resources... Now I understand why you were setting the ID the way you did in the import method. I reverted your last commit, added a diff suppress to the name field and I also fixed the import acceptance test.

Thanks for your contribution!

@rosbo
Copy link
Contributor

rosbo commented Sep 11, 2017

TF_ACC=1 go test ./google -v -run TestAccPubsubTopic -timeout 120m
=== RUN   TestAccPubsubTopic_import
--- PASS: TestAccPubsubTopic_import (2.70s)
=== RUN   TestAccPubsubTopicCreate
--- PASS: TestAccPubsubTopicCreate (2.25s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	5.091s

@rosbo rosbo merged commit bf51f26 into hashicorp:master Sep 11, 2017
@drzero42 drzero42 deleted the pubsub_importable branch September 11, 2017 20:11
@danawillow
Copy link
Contributor

FYI this broke a different test:

------- Stdout: -------
=== RUN   TestAccPubsubSubscriptionCreate
--- FAIL: TestAccPubsubSubscriptionCreate (5.55s)
    testing.go:434: Step 0 error: After applying this step and refreshing, the plan was not empty:
        
        DIFF:
        
        DESTROY/CREATE: google_pubsub_subscription.foobar_sub
          ack_deadline_seconds: "20" => "20"
          name:                 "pssub-test-pmz09wu8bo" => "pssub-test-pmz09wu8bo"
          path:                 "projects/*******/subscriptions/pssub-test-pmz09wu8bo" => "<computed>"
          topic:                "pssub-test-6rnbrodpv7" => "projects/*******/topics/pssub-test-6rnbrodpv7" (forces new resource)
        
        STATE:
        
        google_pubsub_subscription.foobar_sub:
          ID = projects/*******/subscriptions/pssub-test-pmz09wu8bo
          ack_deadline_seconds = 20
          name = pssub-test-pmz09wu8bo
          path = projects/*******/subscriptions/pssub-test-pmz09wu8bo
          topic = pssub-test-6rnbrodpv7
        
          Dependencies:
            google_pubsub_topic.foobar_sub
        google_pubsub_topic.foobar_sub:
          ID = projects/*******/topics/pssub-test-6rnbrodpv7
          name = projects/*******/topics/pssub-test-6rnbrodpv7
FAIL

The answer might just be a DiffSuppress within pubsub subscription but I didn't get around to really investigating this one.

@rosbo
Copy link
Contributor

rosbo commented Sep 13, 2017

Filled #423 for it. I will take a look

negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019
@ghost
Copy link

ghost commented Mar 31, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 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.

4 participants