-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add data source for pubsub topics #7426
Add data source for pubsub topics #7426
Conversation
0af321b
to
c9def16
Compare
c9def16
to
11f1bbf
Compare
11f1bbf
to
6ea06a8
Compare
@ScottSuarez, since this had your name on it before, want to do a first pass code review on it? Once we both decide it looks good, you can also take care of upstreaming it since that's something you should learn how to do. |
Looked over the pull request and things look good to me! Nice job on this mopp :) ! Only comment I had would've been whether or not the checks were necessary on the tests. I didn't think they would've been required as the terraform sdk should ensure that, correct? @danawillow Pending dana's response and any other feedback from her this should be ready to swim on upstream. |
@danawillow @ScottSuarez Thanks for the responses and review. 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! The test checks in this case are good- they're unnecessary for regular resource tests that have import steps, but in this case since there's no built-in ImportStateVerify
-equivalent for data sources, we use checkDataSourceStateMatchesResourceState
.
@ScottSuarez, upstreaming instructions are at https://github.com/hashicorp/terraform-provider-google/blob/master/.github/CONTRIBUTING.md#upstreaming-community-prs-to-magic-modules (this should probably be moved to the wiki tbh). Since the website sidebar is generated, you'll probably need to ignore that part of the change when doing the upstream (it's fine to leave in this PR since the generated output should match).
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! |
What I did
Add data source for Cloud Pub/Sub Topic.
Background
I found the issue #6800
The issue was assigned to @ScottSuarez but it was 26 days ago.
Also, I found a similar PR #7388
The tasks looked not so hard. Then, I tried to create PR.
I'm sorry if I violate some guidelines 🙏
Test
I read the test section of the contribution guideline .
Unfortunately, I don't have GCP project for the test.
I believe the Travis CI runs the tests.
Please tell me if something is wrong.
Closes: #6800