Skip to content

Conversation

geryonghost
Copy link

The push_config for resource "google_pubsub_subscription" "push_subscriptions"doesn't include the no_wrapper settings. This has been added.

@geryonghost geryonghost marked this pull request as ready for review January 17, 2025 19:59
@geryonghost geryonghost requested review from a team and imrannayer as code owners January 17, 2025 19:59
@dheitepriem
Copy link

Hi @imrannayer , @apeabody

did anyone of you already had the opportunity to look at this PR? Getting the no_wrapper setting into the official module would enable us to switch to upstream again from our internal forked version :)

@geryonghost geryonghost requested a review from a team as a code owner January 31, 2025 13:58
@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

Hi @imrannayer , @apeabody

did anyone of you already had the opportunity to look at this PR? Getting the no_wrapper setting into the official module would enable us to switch to upstream again from our internal forked version :)

Thanks for the contribution @geryonghost!

I've updated against main, and triggered the CI tests. Either @imrannayer or a member of @api-pubsub-and-pubsublite should provide the final review.

Cheers!

@apeabody
Copy link
Contributor

apeabody commented Mar 5, 2025

/gcbrun

@apeabody
Copy link
Contributor

apeabody commented Mar 5, 2025

/gcbrun

Copy link
Contributor

@apeabody apeabody 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 the contribution @geryonghost! A few quick notes:

geryonghost and others added 2 commits March 6, 2025 10:17
Co-authored-by: Andrew Peabody <[email protected]>
Co-authored-by: Andrew Peabody <[email protected]>
@geryonghost geryonghost requested a review from apeabody March 6, 2025 16:18
@apeabody
Copy link
Contributor

apeabody commented Mar 6, 2025

/gcbrun

@apeabody
Copy link
Contributor

apeabody commented Mar 6, 2025

Thanks @geryonghost - This change looks good with the existing tests, can you please update one of the examples to use the new setting, so they are also testing with something other than just the default null. Once that is good, we should be ready to merge. Thanks!

@geryonghost
Copy link
Author

@apeabody good morning, I would like to get this PR merged. It has been quite a while. This is my first community submission so I am still learning how to get this done. I fear to update the branch because that always break terraform-google-pubsub-int-trigger (cloud-foundation-cicd) check. What can I do?

@apeabody apeabody requested a review from ayushmjain as a code owner April 11, 2025 16:11
@apeabody apeabody requested a review from q2w as a code owner April 11, 2025 16:11
@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

The linter appears stuck - going to close and re-open the PR

@apeabody apeabody closed this Apr 11, 2025
@apeabody apeabody reopened this Apr 11, 2025
@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

@apeabody good morning, I would like to get this PR merged. It has been quite a while. This is my first community submission so I am still learning how to get this done. I fear to update the branch because that always break terraform-google-pubsub-int-trigger (cloud-foundation-cicd) check. What can I do?

Hi @geryonghost!

The initial test "failed" behavior is expected as the tests need to be triggered by a member. I've updated the branch and triggered the tests which are now running. Either @imrannayer or a member of @api-pubsub-and-pubsublite should provide the final review.

Thanks!

@apeabody apeabody dismissed their stale review April 11, 2025 16:17

My comments have been addressed, pending final review and approval by a repo maintainer. Thanks!

@geryonghost
Copy link
Author

:bump: for a repo maintainer

@geryonghost
Copy link
Author

@ayushmjain, @imrannayer, @q2w, @terraform-google-modules/api-pubsub-and-pubsublite, and/or @terraform-google-modules/cft-admins can you look at this please.

@geryonghost geryonghost deleted the issue-208_missing_no_wrapper_option branch September 18, 2025 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants