Replace "Pubsub Subscription Different Project" example with making all examples cross-project-friendly #9637

glasser · 2023-12-13T18:57:15Z

PR #2342 changed this example from using id to name, which removed the project ID from the topic. However, the point of this example is to show how to set up a cross-project subscription, so the example no longer works.

This change removes that example and uses the id pattern on all the examples, because we don't need a special "cross project" example if the default usage works cross-project. It also expands the topic attribute's description.

Fixes hashicorp/terraform-provider-google#11642.
Fixes hashicorp/terraform-provider-google#6024.

Release Note Template for Downstream PRs (will be copied)

PR GoogleCloudPlatform#2342 changed this example from using `id` to `name`, which removed the project ID from the topic. However, the point of this example is to show how to set up a cross-project subscription, so the example no longer works. This change reverts that change from PR GoogleCloudPlatform#2342. Fixes hashicorp/terraform-provider-google#11642. Fixes hashicorp/terraform-provider-google#6024.

modular-magician · 2023-12-13T18:58:46Z

Hello! I am a robot. It looks like you are a: Community Contributor ~~Googler~~ ~~Core Contributor~~. Tests will require approval to run.

@melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

melinath

We've actually flipped positions on the question of name vs id and now prefer id in all cases; would you be willing to update the other pubsub subscription examples as well?

melinath · 2023-12-13T22:43:30Z

mmv1/templates/terraform/examples/pubsub_subscription_different_project.tf.erb

@@ -6,5 +6,5 @@ resource "google_pubsub_topic" "<%= ctx[:primary_resource_id] %>" {
 resource "google_pubsub_subscription" "<%= ctx[:primary_resource_id] %>" {
  project = "<%= ctx[:vars]['subscription_project'] %>"
  name    = "<%= ctx[:vars]['subscription_name'] %>"
-  topic   = google_pubsub_topic.<%= ctx[:primary_resource_id] %>.name
+  topic   = google_pubsub_topic.<%= ctx[:primary_resource_id] %>.id


To make sure this works as expected, could you extend the example to include setting up a second that's being accessed and remove skip_test?

Sorry, I'm not sure what you mean by "a second" here.

wow, yeah, that sentence is incomprehensible - sorry about that! I meant a second project. If we were running this as a test, we'd want to make sure that we were actually setting up a second project (in addition to the default test project) and that the topic was in the other project. Currently the example uses topic-project but doesn't actually create that project.

But as you pointed out, if the rest of the pubsub documentation uses .id consistently, then cross-project doesn't need to be documented at all since it won't work differently than the other cases.

glasser

I'm not sure I follow your overall suggestion. Are you saying you just think the docs should only document using id rather than name? In that case it probably doesn't need to have this example at all (but maybe the text of the topic option documentation could mention that while you can provide a short name, it's preferred to provide a full id and the latter is necessary for cross-project subscriptions)?

glasser · 2023-12-13T23:28:27Z

mmv1/templates/terraform/examples/pubsub_subscription_different_project.tf.erb

@@ -6,5 +6,5 @@ resource "google_pubsub_topic" "<%= ctx[:primary_resource_id] %>" {
 resource "google_pubsub_subscription" "<%= ctx[:primary_resource_id] %>" {
  project = "<%= ctx[:vars]['subscription_project'] %>"
  name    = "<%= ctx[:vars]['subscription_name'] %>"
-  topic   = google_pubsub_topic.<%= ctx[:primary_resource_id] %>.name
+  topic   = google_pubsub_topic.<%= ctx[:primary_resource_id] %>.id


Sorry, I'm not sure what you mean by "a second" here.

melinath

just to say it explicitly - it seems like it would make sense to remove this example altogether and update the remaining google_pubsub_subscription examples to use .id instead of .name - would you be down to make that change?

glasser · 2023-12-14T04:58:16Z

OK, how's this look?

melinath

Looks good apart from one typo.

mmv1/templates/terraform/examples/pubsub_subscription_dead_letter.tf.erb

modular-magician · 2023-12-14T19:31:20Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 18 insertions(+), 29 deletions(-))
Terraform Beta: Diff ( 3 files changed, 18 insertions(+), 29 deletions(-))
TF OiCS: Diff ( 6 files changed, 6 insertions(+), 6 deletions(-))

modular-magician · 2023-12-14T20:28:32Z

Tests analytics

Total tests: 3293
Passed tests 2955
Skipped tests: 337
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccDataprocClusterIamPolicy

Get to know how VCR tests work

modular-magician · 2023-12-14T20:34:48Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

melinath

LGTM, thanks!

…ll examples cross-project-friendly (GoogleCloudPlatform#9637) * Fix "Pubsub Subscription Different Project" example PR GoogleCloudPlatform#2342 changed this example from using `id` to `name`, which removed the project ID from the topic. However, the point of this example is to show how to set up a cross-project subscription, so the example no longer works. This change reverts that change from PR GoogleCloudPlatform#2342. Fixes hashicorp/terraform-provider-google#11642. Fixes hashicorp/terraform-provider-google#6024. * Make all examples cross-project-friendly and update topic description * Fix typo --------- Co-authored-by: Stephen Lewis (Burrows) <[email protected]>

modular-magician requested a review from melinath December 13, 2023 18:58

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 13, 2023

glasser mentioned this pull request Dec 13, 2023

Bad Doc: Example Usage - Pubsub Subscription Different Project hashicorp/terraform-provider-google#11642

Closed

melinath requested changes Dec 13, 2023

View reviewed changes

glasser commented Dec 13, 2023

View reviewed changes

melinath requested changes Dec 13, 2023

View reviewed changes

Make all examples cross-project-friendly and update topic description

eb9dd4b

glasser changed the title ~~Fix "Pubsub Subscription Different Project" example~~ Replace "Pubsub Subscription Different Project" example with making all examples cross-rpoject-friendly Dec 14, 2023

glasser changed the title ~~Replace "Pubsub Subscription Different Project" example with making all examples cross-rpoject-friendly~~ Replace "Pubsub Subscription Different Project" example with making all examples cross-project-friendly Dec 14, 2023

modular-magician requested a review from melinath December 14, 2023 04:57

melinath approved these changes Dec 14, 2023

View reviewed changes

mmv1/templates/terraform/examples/pubsub_subscription_dead_letter.tf.erb Outdated Show resolved Hide resolved

Fix typo

0318572

modular-magician requested a review from melinath December 14, 2023 17:29

modular-magician added service/pubsub and removed awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests labels Dec 14, 2023

melinath approved these changes Dec 14, 2023

View reviewed changes

melinath merged commit b3ae8dc into GoogleCloudPlatform:main Dec 14, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace "Pubsub Subscription Different Project" example with making all examples cross-project-friendly #9637

Replace "Pubsub Subscription Different Project" example with making all examples cross-project-friendly #9637

glasser commented Dec 13, 2023 •

edited

Loading

modular-magician commented Dec 13, 2023

melinath left a comment

melinath Dec 13, 2023

glasser Dec 13, 2023

melinath Dec 13, 2023

glasser left a comment

glasser Dec 13, 2023

melinath left a comment •

edited

Loading

glasser commented Dec 14, 2023

melinath left a comment

modular-magician commented Dec 14, 2023

modular-magician commented Dec 14, 2023

modular-magician commented Dec 14, 2023

melinath left a comment

Replace "Pubsub Subscription Different Project" example with making all examples cross-project-friendly #9637

Replace "Pubsub Subscription Different Project" example with making all examples cross-project-friendly #9637

Conversation

glasser commented Dec 13, 2023 • edited Loading

modular-magician commented Dec 13, 2023

melinath left a comment

Choose a reason for hiding this comment

melinath Dec 13, 2023

Choose a reason for hiding this comment

glasser Dec 13, 2023

Choose a reason for hiding this comment

melinath Dec 13, 2023

Choose a reason for hiding this comment

glasser left a comment

Choose a reason for hiding this comment

glasser Dec 13, 2023

Choose a reason for hiding this comment

melinath left a comment • edited Loading

Choose a reason for hiding this comment

glasser commented Dec 14, 2023

melinath left a comment

Choose a reason for hiding this comment

modular-magician commented Dec 14, 2023

Diff report

modular-magician commented Dec 14, 2023

Tests analytics

Action taken

modular-magician commented Dec 14, 2023

Rerun these tests in REPLAYING mode to catch issues

melinath left a comment

Choose a reason for hiding this comment

glasser commented Dec 13, 2023 •

edited

Loading

melinath left a comment •

edited

Loading