-
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 Cloud Task Queue resource #4529
Add Cloud Task Queue resource #4529
Conversation
Linter failed in travis. Running locally with go 1.11.13 confirms that it's failing on code I didn't touch. |
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.
lgtm 👍
} | ||
|
||
func resourceTaskQueueImportState(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { | ||
parts := strings.Split(d.Id(), "/") |
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.
Consider doing what resource_kms_key_ring.go does for its import, using parseImportId() and replaceVars().
google/resource_task_queue.go
Outdated
}, | ||
}, | ||
}, | ||
"retry": { |
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.
I would prefer if this matched the API: retry_config
google/resource_task_queue_test.go
Outdated
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckTaskQueueExists( | ||
"google_task_queue.fizzbuzz", location, queueName, &queue), | ||
testAccCheckTaskQueueEquals( | ||
"google_task_queue.fizzbuzz", &queue), | ||
), |
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.
I don't think we need these checks (nor the funcs, and I think that also means we can avoid vendoring this new context
dependency) - the ImportStateVerify
is sufficient.
@tysen, I've removed those tests and renamed |
google/resource_task_queue_test.go
Outdated
|
||
expectedName := fmt.Sprintf("projects/%s/locations/%s/queues/%s", config.Project, attributes["location"], attributes["name"]) | ||
|
||
_, err := config.clientCloudTasks.Projects.Locations.Queues.Get(expectedName).Context(context.Background()).Do() |
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.
Why include this call to Context
? Does this not work without it like the other services?
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.
Sorry for the delay, I was on vacation. I've removed the call to Context
.
I'd love to start using this. What is holding it up? Can I help? |
Still waiting for a response to the question in my review. |
I would really love to use it |
PR looks good now. If you could submit separate PRs (one for TPG and one for TPGB) to vendor the new dependency (and then rebase this PR to remove the vendoring here), afterwards I can upstream this PR into MM and get it merged. |
d7f79b2
to
fbfcea0
Compare
fbfcea0
to
070e0bf
Compare
@tysen, I've rebased this branch and opened #4781 |
@mccall114 Looks like you need to sign the CLA GoogleCloudPlatform/magic-modules#2587 |
@tysen I just signed it |
@mccall114 Thanks, will you post |
any updates on this? |
I had some issues finalizing this implementation, and it'll be easier to maintain and extend if it's generated by Magic Modules, so I submitted GoogleCloudPlatform/magic-modules#2662 which is under review. |
Should be in the next release: GoogleCloudPlatform/magic-modules#2662 |
Thanks @tysen! |
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
New resource
google_task_queue
as part of issue #2367.First time contributor here. I read the guidelines about splitting out new packages in a separate PR, and I'm happy to do that if this one is ok.