10  Writing review comments

A good review comment is specific, constructive, and clear about what is being asked. This chapter explains how to write effective review comments for Terraform pull requests, and shows examples of both blocking and non-blocking feedback.

10.1 Blocking versus non-blocking

Not all issues are equally important. Be clear about the severity of your feedback so the author knows what must be fixed before the PR can merge, and what is a suggestion they can take or leave.

A common convention is to prefix comments with a label:

  • Blocking — this must be fixed before the PR is approved
  • Suggestion — this would improve the code but is not required for approval
  • Question — you want to understand something before deciding whether it is a problem
  • Nit — a minor style or preference point; the author can decide

You do not need to use these exact labels — any clear signal works. What matters is that the author is not left guessing whether a comment requires action.

10.2 What makes a good comment

A useful review comment:

  1. Identifies the specific problem, not just that something is wrong
  2. Explains why it is a problem — the reasoning, not just the judgement
  3. Suggests what to do instead where possible
  4. Distinguishes required changes from suggestions

A comment that just says “this is wrong” is not helpful. The author cannot act on it, and it does not help them learn.

10.3 Example comments

Hardcoded project ID — blocking

Blocking: The project ID my-project-12345 is hardcoded here. If this module is called from a different environment, it will point to the wrong project. Please pass the project ID in via a variable, or use var.project_id if it is already declared.

Overly permissive IAM role — blocking

Blocking: roles/editor grants write access to almost every resource in the project. This is much broader than the service account needs. Can you check the documentation for the specific service being used and find a narrower predefined role? For example, if this service account only needs to read from Cloud Storage, roles/storage.objectViewer would be appropriate.

Missing prevent_destroy on a database — blocking (for production)

Blocking: This Cloud SQL instance does not have prevent_destroy = true in its lifecycle block. In a production environment, an accidental terraform destroy or a resource replacement could cause data loss. Please add:

lifecycle {
  prevent_destroy = true
}

google_project_iam_binding used where member was likely intended — blocking

Blocking: google_project_iam_binding is authoritative — it will remove any other members from roles/cloudsql.client that are not listed in this resource. If there are other service accounts or users that currently have this role, they will lose access on the next apply. Was this intentional? If you only want to add this one member without affecting others, use google_project_iam_member instead.

Missing variable description — suggestion

Suggestion: The variable enable_versioning has no description. Can you add one? It makes the code easier to understand and feeds into generated module documentation:

variable "enable_versioning" {
  description = "Whether to enable object versioning on the storage bucket."
  type        = bool
  default     = false
}

Suppressed checkov finding without explanation — blocking

Blocking: CKV_GCP_62 (access logging) has been suppressed on this bucket with no explanation. For a bucket in a non-production environment, this may be acceptable, but please add a comment explaining why:

#checkov:skip=CKV_GCP_62:Access logging not required for ephemeral test fixture bucket

If access logging should be enabled here, please remove the suppression and configure logging.

Question about a lifecycle rule

Question: I see ignore_changes = [labels] on this resource. Is this because labels are being managed outside Terraform? If so, it would be helpful to add a comment explaining that, so the next person who looks at this does not assume it was forgotten.

Non-obvious configuration — nit

Nit: The uniform_bucket_level_access = true setting is correct here, but it might be worth a brief comment to explain that this is set by our data handling policy, not just as a default. Helps future reviewers understand why it is explicitly set.

10.4 Tone

Review comments should be collegial and focused on the code, not the person.

  • “This variable is undescribed” is better than “you forgot to describe this variable”
  • “This role is too broad” is better than “why did you use such a wide role?”
  • Offer context and suggestions, not just problems

Infrastructure review often involves calling out potential security or data integrity issues. That is a healthy part of the process — not a criticism of the author’s ability.

10.5 When you are not sure

If you see something you do not fully understand, ask rather than guessing. A question is a legitimate and valuable review comment:

Question: I am not familiar with this resource type. The google_compute_backend_service with load_balancing_scheme = "INTERNAL" — is this used for internal load balancing only, or can it receive external traffic? Want to make sure I understand the network exposure here.

Asking a question is not a sign of inexperience. It often surfaces important context that should be in the PR description or code comments anyway.