added review a PR guide

[shuyama1]

fixes b/288626199

If this PR is for Terraform, I acknowledge that I have:

• Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
• Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
• Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
• Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
• Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

[modular-magician]

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

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

[modular-magician]

Tests analytics

Total tests: 2834
Passed tests 2538
Skipped tests: 295
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

TestAccComputeFirewallPolicyRule_multipleRules

Get to know how VCR tests work

[modular-magician]

Tests failed during RECORDING mode:
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

[modular-magician]

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

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

[rileykarson]

There are different expectations for maintainer and contributor PRs (self-service / community), should we call them out separately in some way? In particular I'm thinking of:

• I don't give more than a passing glance to release notes for a maintainer PR, and trust they've written them correctly. For contributor PRs I've got to author them myself.
• For contributors I've got to wait until all tests are green until clicking approve. For maintainers, I'm generally fine approving if I'm satisfied with the code as-written and tests are most of the way (80-90%) there, even if they're currently failing.

[shuyama1]

I agree that we have different expectations for maintainer and contributor PRs, but I think this review guide is more like a general checklist that users/ reviewers should follow to (self-)review a PR. We should have all these boxes checked before merging a PR no matter it's a maintainer or contributor PR. Although for maintainer PRs, it's more like the PR author's responsibility to ensure all these steps have been checked. I could call out that that it's author's responsibility (or whoever click the merge button) to ensure that the PR passed all the checks on this list for maintainer PRs. What's your thought?

[melinath]

FWIW I would prefer not to call out that we might have separate standards for maintainers since that is not relevant to most of the people who would read this page and might confuse folks

[rileykarson]

Ehh, fair enough- the page is doing double-duty as a maintainer and contributor guide in that case, which may leave ambiguities for maintainers.

[modular-magician]

Tests analytics

Total tests: 2836
Passed tests 2540
Skipped tests: 295
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

TestAccComputeFirewallPolicyRule_multipleRules

Get to know how VCR tests work

[modular-magician]

Tests failed during RECORDING mode:
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

[shuyama1]

updated version:

[modular-magician]

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

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

[modular-magician]

Tests analytics

Total tests: 2847
Passed tests 2550
Skipped tests: 295
Affected tests: 2

Action taken

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

TestAccComputeFirewallPolicyRule_multipleRules|TestAccContainerAwsNodePool_BetaBasicHandWritten

Get to know how VCR tests work

[modular-magician]

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]
TestAccContainerAwsNodePool_BetaBasicHandWritten[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

[rileykarson]

Any thoughts on different responsibilities as mentioned in #8258 (comment)?