Conversation
omendra-tomar
requested review from
anshulsao,
ishaankalra,
pramodh-ayyappan,
rr0hit,
unni-facets and
vishnukv-facets
as code owners
WalkthroughAdds a new Google Cloud Pub/Sub Terraform module (default flavor v1.0): README, facets.yaml, variables, shared locals, topic resource, optional schema, simplified pull and push subscriptions, and outputs; resources use environment data for naming, labels, and provider/project resolution. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Facet as facets.yaml
participant TF as Pub/Sub Module
participant GCP as GCP API
Dev->>Facet: Provide instance spec (topic, subs, retention)
Facet->>TF: Validate & forward inputs
TF->>TF: Compute env-suffixed names, labels, locals
TF->>GCP: Create Topic
alt schema enabled and defined
TF->>GCP: Create Schema
end
par create subscriptions
TF->>GCP: Create Pull Subscriptions (for_each)
TF->>GCP: Create Push Subscriptions (for_each with push_endpoint)
end
TF-->>Dev: Return outputs (topic id/name, sub names, labels)
note right of TF: Project resolved from inputs.cloud_account.project or provider default
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (9)
modules/pubsub/default/1.0/README.md (1)
24-29: Fix resource list phrasing (minor terminology/grammar).
- “Protocol Buffer” → “Protocol Buffers”
- “Client-pull based” → “Client pull‑based”
- Consider “Dead‑letter topics” (GCP docs commonly say “dead letter”/“dead-letter”).
Apply:
- - **Pub/Sub Schema** - Optional message validation schema supporting AVRO and Protocol Buffer formats - - **Pull Subscriptions** - Client-pull based subscriptions with configurable acknowledgment settings + - **Pub/Sub Schema** - Optional message validation schema supporting AVRO and Protocol Buffers + - **Pull Subscriptions** - Client pull‑based subscriptions with configurable acknowledgment settings - - **Dead Letter Topics** - Automatic handling of failed message delivery (when configured) + - **Dead‑letter topics** - Automatic handling of failed message delivery (when configured)modules/pubsub/default/1.0/variables.tf (1)
1-24: Confirm Terraform/core provider versions supportoptional()andalltrue().These require modern Terraform (>= 1.3). Add a
terraformblock withrequired_versionandrequired_providersin this module to make the constraint explicit.I can add a
versions.tfwith pinned constraints if you confirm your pipeline versions.modules/pubsub/default/1.0/topic.tf (1)
16-18: Validate/normalize retention duration.Ensure
local.topic_message_retention_durationconforms to Pub/Sub's format and limits. Consider adding a variable validation regex like^\d+s$with acceptable min/max.I can add a regex validation in variables.tf if desired.
modules/pubsub/default/1.0/outputs.tf (1)
6-7: Use fully-qualified topic name for “topic_full_name”.
google_pubsub_topic.this.idreturnsprojects/{project}/topics/{name};nameis just the short name.Apply:
- topic_full_name = google_pubsub_topic.this.name + topic_full_name = google_pubsub_topic.this.idmodules/pubsub/default/1.0/schema.tf (1)
16-17: Remove or wire upschema_encoding
modules/pubsub/default/1.0/schema.tf line 16 definesschema_encoding, but it isn’t referenced anywhere in this module. Either hook it intotopic.schema_settingsin topic.tf or remove it to avoid drift.modules/pubsub/default/1.0/push_subscriptions.tf (1)
21-40: Consider minimal resilience knobs (optional)Optional but useful in most prod setups:
retry_policy,dead_letter_policy, andfilter. Even if “simplified,” exposing these as optional fields prevents later breaking changes.modules/pubsub/default/1.0/facets.yaml (1)
50-71: Optional: Add OIDC config to push subscriptionsPrepares the facet for authenticated push.
Apply:
^[a-zA-Z0-9_-]+$: type: object title: Push Subscription Configuration properties: push_endpoint: type: string title: Push Endpoint description: The URL where messages will be pushed + oidc: + type: object + title: OIDC Token (optional) + properties: + service_account_email: + type: string + title: Service Account Email + audience: + type: string + title: Audience (optional) ack_deadline_seconds: type: integer title: Acknowledgment Deadline (seconds) description: Time before Pub/Sub attempts redelivery default: 20 minimum: 10 maximum: 600modules/pubsub/default/1.0/pull_subscriptions.tf (2)
11-18: Minor: defensive defaults and validationIf
ack_deadline_secondsis omitted, this rendersnull. Consider defaulting to 20 here to mirror the facet default.Apply:
- ack_deadline_seconds = config.ack_deadline_seconds + ack_deadline_seconds = try(config.ack_deadline_seconds, 20)
21-33: Optional resiliency knobsConsider optional
retry_policyanddead_letter_policyto prevent message loss/backlogs in prod.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
modules/pubsub/default/1.0/README.md(1 hunks)modules/pubsub/default/1.0/facets.yaml(1 hunks)modules/pubsub/default/1.0/main.tf(1 hunks)modules/pubsub/default/1.0/outputs.tf(1 hunks)modules/pubsub/default/1.0/pull_subscriptions.tf(1 hunks)modules/pubsub/default/1.0/push_subscriptions.tf(1 hunks)modules/pubsub/default/1.0/schema.tf(1 hunks)modules/pubsub/default/1.0/topic.tf(1 hunks)modules/pubsub/default/1.0/variables.tf(1 hunks)
🧰 Additional context used
🪛 LanguageTool
modules/pubsub/default/1.0/README.md
[grammar] ~24-~24: There might be a mistake here.
Context: ...le retention and optional KMS encryption - Pub/Sub Schema - Optional message vali...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...porting AVRO and Protocol Buffer formats - Pull Subscriptions - Client-pull based...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...ith configurable acknowledgment settings - Push Subscriptions - HTTP endpoint pus...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...ions with retry and dead letter policies - Dead Letter Topics - Automatic handlin...
(QB_NEW_EN)
| properties: | ||
| push_endpoint: | ||
| type: string | ||
| title: Push Endpoint | ||
| description: The URL where messages will be pushed | ||
| ack_deadline_seconds: | ||
| type: number | ||
| title: Acknowledgment Deadline (seconds) | ||
| description: Time before Pub/Sub attempts redelivery | ||
| default: 20 | ||
| minimum: 10 | ||
| maximum: 600 |
There was a problem hiding this comment.
Make push_endpoint required for push subscriptions
Without this, the module can render invalid resources.
Apply:
patternProperties:
^[a-zA-Z0-9_-]+$:
type: object
title: Push Subscription Configuration
properties:
push_endpoint:
type: string
title: Push Endpoint
description: The URL where messages will be pushed
ack_deadline_seconds:
- type: number
+ type: integer
title: Acknowledgment Deadline (seconds)
description: Time before Pub/Sub attempts redelivery
default: 20
minimum: 10
maximum: 600
+ required:
+ - push_endpoint📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| properties: | |
| push_endpoint: | |
| type: string | |
| title: Push Endpoint | |
| description: The URL where messages will be pushed | |
| ack_deadline_seconds: | |
| type: number | |
| title: Acknowledgment Deadline (seconds) | |
| description: Time before Pub/Sub attempts redelivery | |
| default: 20 | |
| minimum: 10 | |
| maximum: 600 | |
| patternProperties: | |
| ^[a-zA-Z0-9_-]+$: | |
| type: object | |
| title: Push Subscription Configuration | |
| properties: | |
| push_endpoint: | |
| type: string | |
| title: Push Endpoint | |
| description: The URL where messages will be pushed | |
| ack_deadline_seconds: | |
| type: integer | |
| title: Acknowledgment Deadline (seconds) | |
| description: Time before Pub/Sub attempts redelivery | |
| default: 20 | |
| minimum: 10 | |
| maximum: 600 | |
| required: | |
| - push_endpoint |
🤖 Prompt for AI Agents
In modules/pubsub/default/1.0/facets.yaml around lines 60 to 71, the
push_endpoint property is currently optional which allows generation of invalid
push subscription resources; update the schema to make push_endpoint required by
adding it to the schema's required array (e.g., add a required entry that
includes "push_endpoint") so the validator/enforcer will demand a push_endpoint
value when creating push subscriptions.
modules/pubsub/default/1.0/main.tf
Outdated
| # Use environment's project automatically - no user input needed | ||
| project_id = null # Let Terraform use the default project from provider/environment | ||
| spec = var.instance.spec |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set project_id from environment to bind all resources.
project_id = null is a no‑op. Pull an explicit value from var.environment and reuse across resources.
Apply:
- project_id = null # Let Terraform use the default project from provider/environment
+ # Prefer explicit scoping; add `project_id` to var.environment and set it here
+ project_id = try(var.environment.project_id, null)Also update variables.tf to include project_id in environment type.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Use environment's project automatically - no user input needed | |
| project_id = null # Let Terraform use the default project from provider/environment | |
| spec = var.instance.spec | |
| # Use environment's project automatically - no user input needed | |
| # Prefer explicit scoping; add `project_id` to var.environment and set it here | |
| project_id = try(var.environment.project_id, null) | |
| spec = var.instance.spec |
🤖 Prompt for AI Agents
In modules/pubsub/default/1.0/main.tf around lines 7-9, replace the no-op
project_id = null with an explicit reference to the environment project ID
(e.g., project_id = var.environment.project_id or a
local.environment_project_id) so the module binds resources to the correct
project, and update modules/pubsub/default/1.0/variables.tf to add project_id to
the environment object/type (and any callers/roots that supply var.environment)
so the value is required/propagated consistently across all resources.
| # Generate topic name from user input and cluster_code | ||
| topic_name = "${var.instance.spec.topic_name}-${var.environment.cluster_code}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize topic name to safe charset and case.
Guard against unexpected chars in cluster_code and enforce lowercase to keep naming consistent.
Apply:
- topic_name = "${var.instance.spec.topic_name}-${var.environment.cluster_code}"
+ topic_name = lower(regexreplace("${var.instance.spec.topic_name}-${var.environment.cluster_code}", "[^a-z0-9._-]", "-"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Generate topic name from user input and cluster_code | |
| topic_name = "${var.instance.spec.topic_name}-${var.environment.cluster_code}" | |
| # Generate topic name from user input and cluster_code | |
| topic_name = lower(regexreplace("${var.instance.spec.topic_name}-${var.environment.cluster_code}", "[^a-z0-9._-]", "-")) |
🤖 Prompt for AI Agents
In modules/pubsub/default/1.0/main.tf around lines 11 to 13, the topic_name
interpolation uses raw values which may contain unexpected characters and mixed
case; normalize both parts by replacing any characters outside [A-Za-z0-9-] with
a safe separator (e.g. "-") and convert the result to lowercase before
concatenation. Update the expression to run regexreplace on
var.instance.spec.topic_name and var.environment.cluster_code to sanitize them,
then wrap the final string with lower() so the produced topic_name is lowercase
and contains only allowed characters.
| locals { | ||
| # Schema-specific configuration | ||
| schema_config = local.schema_enabled && lookup(local.spec, "schema", null) != null ? local.spec.schema : null | ||
|
|
||
| # Schema name with environment suffix (only when schema is enabled and name is provided) | ||
| schema_name = local.schema_enabled && local.schema_config != null ? lookup(local.schema_config, "name", null) != null ? "${local.schema_config.name}-${var.environment.cluster_code}" : null : null | ||
|
|
||
| # Schema properties | ||
| schema_type = local.schema_enabled && local.schema_config != null ? lookup(local.schema_config, "type", null) : null | ||
| schema_definition = local.schema_enabled && local.schema_config != null ? lookup(local.schema_config, "definition", null) : null | ||
| schema_encoding = local.schema_enabled && local.schema_config != null ? lookup(local.schema_config, "encoding", null) : null | ||
| } |
There was a problem hiding this comment.
Mismatch: spec has no schema block; this code expects local.spec.schema
locals.schema_config reads local.spec.schema, but facets.yaml defines no schema property, making schema creation unreachable.
I’ve proposed the facets.yaml additions in its file comment.
| # Create Pub/Sub topic - uses environment's default project | ||
| resource "google_pubsub_topic" "this" { | ||
| name = local.topic_name | ||
| labels = local.filtered_labels | ||
|
|
||
| # Topic configuration | ||
| message_retention_duration = local.topic_message_retention_duration | ||
|
|
||
| # No project specified - uses default from provider/environment | ||
| # No schema support in basic version | ||
| # No KMS encryption in basic version | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Explicitly set project to avoid accidental cross‑project applies.
Current resource relies on provider default project. If multiple providers are used, this is risky.
Apply:
resource "google_pubsub_topic" "this" {
- name = local.topic_name
- labels = local.filtered_labels
+ project = local.project_id
+ name = local.topic_name
+ labels = local.filtered_labelsFollow‑up: wire local.project_id (see main.tf comment) to an explicit environment project.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Create Pub/Sub topic - uses environment's default project | |
| resource "google_pubsub_topic" "this" { | |
| name = local.topic_name | |
| labels = local.filtered_labels | |
| # Topic configuration | |
| message_retention_duration = local.topic_message_retention_duration | |
| # No project specified - uses default from provider/environment | |
| # No schema support in basic version | |
| # No KMS encryption in basic version | |
| } | |
| resource "google_pubsub_topic" "this" { | |
| project = local.project_id | |
| name = local.topic_name | |
| labels = local.filtered_labels | |
| # Topic configuration | |
| message_retention_duration = local.topic_message_retention_duration | |
| # No schema support in basic version | |
| # No KMS encryption in basic version | |
| } |
🤖 Prompt for AI Agents
In modules/pubsub/default/1.0/topic.tf around lines 11 to 22, the
google_pubsub_topic resource omits the project attribute and relies on the
provider default which can cause accidental cross‑project applies; add a project
= local.project_id line to the resource block (wiring the local.project_id
referenced in main.tf) so the topic is created in the intended environment
project and not whatever provider default is active.
| topic_name = optional(string, "my-topic") | ||
| topic_message_retention_duration = optional(string, "604800s") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for topic name format and length.
Pub/Sub names have character and length constraints. Validate early to avoid plan/apply failures.
Apply:
spec = object({
# Topic Configuration
- topic_name = optional(string, "my-topic")
+ topic_name = optional(string, "my-topic")
topic_message_retention_duration = optional(string, "604800s")And append after Line 41:
+ # Topic name validations
+ validation {
+ condition = can(regex("^[-a-zA-Z0-9._]{3,255}$", var.instance.spec.topic_name))
+ error_message = "topic_name must be 3–255 chars; allowed: letters, numbers, hyphen, underscore, period."
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| topic_name = optional(string, "my-topic") | |
| topic_message_retention_duration = optional(string, "604800s") | |
| spec = object({ | |
| # Topic Configuration | |
| topic_name = optional(string, "my-topic") | |
| topic_message_retention_duration = optional(string, "604800s") | |
| }) | |
| # Topic name validations | |
| validation { | |
| condition = can(regex("^[-a-zA-Z0-9._]{3,255}$", var.instance.spec.topic_name)) | |
| error_message = "topic_name must be 3–255 chars; allowed: letters, numbers, hyphen, underscore, period." | |
| } |
🤖 Prompt for AI Agents
In modules/pubsub/default/1.0/variables.tf around lines 9-11 and append after
line 41, add validation for the topic_name variable so Terraform fails fast on
bad values: require topic_name length between 3 and 255 characters and enforce
allowed Pub/Sub characters via a regex (e.g. ^[A-Za-z0-9-_.~+%]+$); implement
this using a validation block on the variable (with a clear error message) so
both default and user-supplied values are checked before plan/apply.
| push_subscriptions = optional(map(object({ | ||
| push_endpoint = string | ||
| ack_deadline_seconds = optional(number, 20) | ||
| })), {}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce HTTPS push endpoints.
Push endpoints should be HTTPS; add validation to prevent insecure configs.
Apply after the existing push validation block:
+ # Push endpoint must be HTTPS
+ validation {
+ condition = alltrue([
+ for _, sub_config in var.instance.spec.push_subscriptions :
+ startswith(lower(sub_config.push_endpoint), "https://")
+ ])
+ error_message = "Push subscription push_endpoint must start with https://"
+ }Also applies to: 34-41
🤖 Prompt for AI Agents
In modules/pubsub/default/1.0/variables.tf around lines 18-21 (and similarly for
lines 34-41), the push_endpoint field currently allows arbitrary URLs; add a
validation block that enforces HTTPS by checking that each push_endpoint starts
with "https://" (or use a regex that requires ^https://) and throw a meaningful
error message when the check fails; place this new validation immediately after
the existing push validation block for each map/object definition so insecure
(http://) endpoints are rejected at plan time.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
modules/pubsub/default/1.0/facets.yaml (3)
22-33: Expose optional schema in facet to match schema.tfschema.tf references spec.schema.*, but the facet has no schema block; this will break schema attachment/config. Add a toggleable schema object.
properties: topic_name: type: string @@ topic_message_retention_duration: type: string title: Message Retention Duration description: How long to retain messages (e.g., '604800s' for 7 days) default: 604800s + schema: + type: object + title: Topic Schema (optional) + description: Configure a Pub/Sub schema and attach it to the topic + x-ui-toggle: true + properties: + name: + type: string + title: Schema Name + description: Schema ID; environment code will be suffixed + type: + type: string + title: Schema Type + enum: [AVRO, PROTOBUF] + definition: + type: string + title: Schema Definition + description: AVRO schema JSON or PROTO file contents + encoding: + type: string + title: Topic Encoding + enum: [JSON, BINARY]
43-49: Use integer type for ack_deadline_seconds (pull and push)Provider expects an integer; this prevents float input slipping through validation.
ack_deadline_seconds: - type: number + type: integer @@ ack_deadline_seconds: - type: number + type: integerAlso applies to: 65-71
56-71: Require push_endpoint for push subscriptionsWithout a push_endpoint, the module can render invalid push subscriptions. Enforce via schema.
patternProperties: ^[a-zA-Z0-9_-]+$: type: object title: Push Subscription Configuration properties: push_endpoint: type: string title: Push Endpoint description: The URL where messages will be pushed ack_deadline_seconds: type: integer title: Acknowledgment Deadline (seconds) description: Time before Pub/Sub attempts redelivery default: 20 minimum: 10 maximum: 600 + required: + - push_endpoint
🧹 Nitpick comments (4)
modules/pubsub/default/1.0/facets.yaml (4)
41-49: Disallow unknown keys in subscription configsPrevents typos silently passing validation.
^(?!goog)[A-Za-z][A-Za-z0-9._~+%-]{2,254}$: type: object title: Pull Subscription Configuration + additionalProperties: false properties: @@ ^(?!goog)[A-Za-z][A-Za-z0-9._~+%-]{2,254}$: type: object title: Push Subscription Configuration + additionalProperties: false properties:Also applies to: 59-71
21-22: Lock top-level spec to known propertiesAvoids stray/unsupported keys at top level.
type: object + additionalProperties: false properties:
32-32: Quote duration scalars for consistency and parser safetyYAML plain scalars are fine, but quoting avoids edge cases and matches examples.
- default: 604800s + default: "604800s" @@ - topic_message_retention_duration: 604800s + topic_message_retention_duration: "604800s"Also applies to: 83-83
28-33: Optionally validate retention duration format and boundsPub/Sub accepts INTEGER with units (s/m/h/d), min 10m, max 31d. Consider a pattern and doc note. (cloud.google.com)
topic_message_retention_duration: type: string title: Message Retention Duration - description: How long to retain messages (e.g., '604800s' for 7 days) + description: How long to retain messages (min 10m, max 31d). Valid forms: INTEGER[UNIT] where UNIT is s, m, h, or d (e.g., '604800s' for 7 days). + pattern: "^[0-9]+[smhd]$" default: "604800s"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
modules/pubsub/default/1.0/facets.yaml(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
modules/pubsub/default/1.0/facets.yaml (4)
39-39: Tighten subscription name regex to Pub/Sub rulesEnforce start-with-letter, 3–255 chars, allowed chars, and not starting with "goog".
- ^[a-zA-Z0-9_-]+$: + ^(?!goog)[A-Za-z][A-Za-z0-9._~+%-]{2,254}$:Apply to both pull and push sections.
Also applies to: 57-57
12-16: Blocking: schema.tf is listed but facet exposes no schema configschema.tf can’t work without
spec.schema.*. Add aschemablock to the facet.@@ properties: @@ topic_message_retention_duration: type: string title: Message Retention Duration description: How long to retain messages (e.g., '604800s' for 7 days) default: 604800s + schema: + type: object + title: Topic Schema (optional) + description: Configure a Pub/Sub schema and optionally attach it to the topic + x-ui-toggle: true + properties: + name: + type: string + title: Schema Name + description: Schema id; environment code will be suffixed + type: + type: string + title: Schema Type + enum: [AVRO, PROTOBUF] + definition: + type: string + title: Schema Definition + description: AVRO schema JSON or PROTO file contents + encoding: + type: string + title: Topic Encoding + enum: [JSON, BINARY]Also applies to: 22-33
43-49: Use integer for ack_deadline_secondsProvider expects an integer; prevent floats at the UI/schema layer.
ack_deadline_seconds: - type: number + type: integer @@ ack_deadline_seconds: - type: number + type: integerAlso applies to: 66-71
56-71: Make push_endpoint required and enforce HTTPSPush subscriptions without HTTPS endpoint will fail; validate early.
patternProperties: - ^[a-zA-Z0-9_-]+$: + ^[a-zA-Z0-9_-]+$: type: object title: Push Subscription Configuration properties: push_endpoint: type: string title: Push Endpoint - description: The URL where messages will be pushed + description: The HTTPS URL where messages will be pushed + format: uri + pattern: '^https://' ack_deadline_seconds: type: integer title: Acknowledgment Deadline (seconds) description: Time before Pub/Sub attempts redelivery default: 20 minimum: 10 maximum: 600 + required: + - push_endpoint
🧹 Nitpick comments (2)
modules/pubsub/default/1.0/facets.yaml (2)
28-33: Quote duration defaults and validate formatEnsure YAML parses as string and guard with a simple seconds+suffix pattern.
topic_message_retention_duration: type: string title: Message Retention Duration description: How long to retain messages (e.g., '604800s' for 7 days) - default: 604800s + default: '604800s' + pattern: '^[0-9]+s$' @@ - topic_message_retention_duration: 604800s + topic_message_retention_duration: '604800s'Also applies to: 84-84
23-27: Validate topic_name against Pub/Sub naming rulesPrevents late failures and enforces consistency.
topic_name: type: string title: Topic Name description: Name for the Pub/Sub topic (will be suffixed with environment code) default: my-topic + pattern: '^(?!goog)[A-Za-z][A-Za-z0-9._~+%-]{2,254}$'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
modules/pubsub/default/1.0/facets.yaml(1 hunks)
🔇 Additional comments (1)
modules/pubsub/default/1.0/facets.yaml (1)
9-16: Consistency checks passed – all listed.tffiles exist; schema is only configured inschema.tfper the default facet (no schema intopic.tf); andack_deadline_secondsis handled in both subscription modules.
ishaankalra
left a comment
ishaankalra
left a comment
There was a problem hiding this comment.
give an input of cloud account
modules/pubsub/default/1.0/main.tf
Outdated
|
|
||
| locals { | ||
| # Use environment's project automatically - no user input needed | ||
| project_id = null # Let Terraform use the default project from provider/environment |
There was a problem hiding this comment.
take optional project id input from the cloud account
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
modules/pubsub/default/1.0/facets.yaml (1)
84-99: Makepush_endpointmandatory for push subscriptions.Without
push_endpoint, the module happily accepts invalid push subscription specs and Terraform fail-fast later. Add arequiredarray so the facet enforces the contract upfront.patternProperties: ^[a-zA-Z0-9_-]+$: type: object title: Push Subscription Configuration properties: push_endpoint: type: string title: Push Endpoint description: The URL where messages will be pushed ack_deadline_seconds: type: integer title: Acknowledgment Deadline (seconds) description: Time before Pub/Sub attempts redelivery default: 20 minimum: 10 maximum: 600 + required: + - push_endpoint
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/pubsub/default/1.0/facets.yaml(1 hunks)modules/pubsub/default/1.0/main.tf(1 hunks)modules/pubsub/default/1.0/variables.tf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/pubsub/default/1.0/main.tf
- modules/pubsub/default/1.0/variables.tf
| description: Pull-based subscriptions where clients pull messages from Pub/Sub | ||
| x-ui-toggle: true | ||
| patternProperties: | ||
| ^[a-zA-Z0-9_-]+$: |
There was a problem hiding this comment.
Tighten subscription name regex to match Pub/Sub rules.
Current pattern allows invalid identifiers (e.g., leading digit, “goog*” prefixes) and rejects valid ones (e.g., containing ~). Mirror Google’s constraints so users don’t get runtime failures.
- ^[a-zA-Z0-9_-]+$:
+ ^(?!goog)[A-Za-z][A-Za-z0-9._~+%-]{2,254}$:Apply the same change to both pull and push subscription keys.
Also applies to: 85-85
2 participants
Description
Related issues
Type of change
Checklist
developbranchTesting
Reviewer instructions
Summary by CodeRabbit
New Features
Documentation