-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 deadLetterPolicy to Pub/Sub Subscription resource #3305
Add deadLetterPolicy to Pub/Sub Subscription resource #3305
Conversation
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @slevenick, please review this PR or find an appropriate assignee. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 170 insertions(+)) |
Oops! It looks like no changelog entry is attached to this PR. Please include a release note block in the PR body, as described in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md:
|
360247a
to
ae9bb38
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 170 insertions(+)) |
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
ae9bb38
to
b8a95cd
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 170 insertions(+)) |
products/pubsub/api.yaml
Outdated
@@ -244,3 +244,44 @@ objects: | |||
If ttl is not set, the associated resource never expires. | |||
A duration in seconds with up to nine fractional digits, terminated by 's'. | |||
Example - "3.5s". | |||
- !ruby/object:Api::Type::NestedObject | |||
name: 'deadLetterPolicy' | |||
allow_empty_object: true |
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.
We probably don't want allow_empty_object
here. It will allow us to specify dead_letter_policy {}
in a config, which is invalid for this.
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.
Fixed.
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.
Thanks for the addition!
Can I have you add an integration test based on dead letter policies? It can be added as a file that is referenced in the products/pubsub/terraform.yaml
examples
block for the pubsub subscription.
products/pubsub/api.yaml
Outdated
is disabled. | ||
|
||
The Cloud Pub/Sub service account associated with this subscriptions's | ||
parent project (i.e.,\nservice-{project_number}@gcp-sa-pubsub.iam.gserviceaccount.com) must have |
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.
Extra \n
in here
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.
Fixed. Example added as well.
ee25841
to
165c604
Compare
165c604
to
3454a2b
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 245 insertions(+)) |
@slevenick I have problem troubleshooting integration test failure:
I'm using Terraform 0.12. |
|
||
expiration_policy { | ||
dead_letter_topic = google_pubsub_topic.<%= ctx[:primary_resource_id] %>_dead_letter.id | ||
max_delivery_attempts = <%= ctx[:vars]['max_delivery_attempts'] %> |
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.
The issue is that this is getting a random suffix added on during generation. Instead of using ctx here, just put the number 10 directly in this config
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.
Fixed
You can see the issue in the downstream generated commit: https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-3305-old..auto-pr-3305#diff-465638d1b1e0cb367d7c78b83d054b32R118 |
@slevenick Thanks for help, I was able to fix it and integration tests are running successfully now. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 245 insertions(+)) |
aab89de
to
e24b664
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 245 insertions(+)) |
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 to me!
Thanks for the addition
…tform#3305) * Add deadLetterPolicy to Pub/Sub subscription resource * fix: disable allow_empty_objects, fix docstring, add example * fix: set max_delivery_attempts * fix: block and topic name
Support dead_letter_policy as specified in https://pubsub.googleapis.com/$discovery/rest?version=v1
I've tested changes by applying dead_letter_policy with locally build terraform provider following instructions in README.
Release Note Template for Downstream PRs (will be copied)
Fixes hashicorp/terraform-provider-google#5522