-
Notifications
You must be signed in to change notification settings - Fork 128
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
feat(push): Add Pub/Sub Subscription support for push payload unwrapping (no wrapper) #141
Conversation
/gcbrun |
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 contribution @ORabbit! I have included some suggestions below. Cheers!
main.tf
Outdated
@@ -195,6 +195,13 @@ resource "google_pubsub_subscription" "push_subscriptions" { | |||
audience = lookup(each.value, "audience", "") | |||
} | |||
} | |||
|
|||
dynamic "no_wrapper" { | |||
for_each = lookup(each.value, "no_wrapper", "") ? [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.
Rather than a bool, would it make more sense to use a nested block no_wrapper = {write_metadata: false}
with a default of null
? That would more closely follow the provider and allow for easy addition of additional keys in the future.
@@ -28,6 +28,8 @@ module "pubsub" { | |||
x-goog-version = "v1beta1" // optional | |||
oidc_service_account_email = "[email protected]" // optional | |||
audience = "example" // optional | |||
no_wrapper = false // optional |
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.
If possible, please add any new variables to an example so they are exercised in the Integration CI: https://github.com/terraform-google-modules/terraform-google-pubsub/tree/master/examples
FYI @ORabbit - The Int CI is returning:
FYI: Your |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
Any way we can reopen this one? We're blocked by it... (or maybe there's a workaround?) |
examples/subscriptions_only/main.tf
Outdated
{ | ||
name = "push_payload_unwrapping" | ||
push_endpoint = "https://${var.project_id}.appspot.com/" | ||
no_wrapper = { write_metadata = 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.
I think we need something like
no_wrapper = true
no_wrapper_write_metadata = true
/gcbrun |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
No description provided.