feat: separate external public/private subnet ids #43

jochem725 · 2025-10-24T08:50:38Z

what

Separate the subnet id parameter into public and private subnet ids

why

In order to spawn both public and private workers when the networking_stack is set to external we need to pass both private and public subnets. A single parameter makes this difficult with atmos; this fix allows easier specification.
More recent versions of runs-on split the subnets as well (https://runs-on.com/configuration/stack-config/#externalvpcpublicsubnetids)

references

Solves https://github.com/orgs/cloudposse/discussions/106

Summary by CodeRabbit

New Features
- Subnet configuration now requires separate public and private subnet parameters instead of a combined list.

coderabbitai · 2025-10-24T08:50:50Z

Walkthrough

The pull request refactors subnet handling in a Terraform module by splitting a single subnet_ids variable into two distinct variables: public_subnet_ids and private_subnet_ids. The main configuration now concatenates these subnets and passes them through a remapped parameter structure.

Changes

Cohort / File(s)	Summary
Variable Restructuring `src/variables.tf`	Replaced single `subnet_ids` variable with two new variables: `public_subnet_ids` (nullable, default null) and `private_subnet_ids` (nullable, default null), both of type `list(string)`
Local and Parameter Wiring `src/main.tf`	Modified `subnet_ids` local to concatenate public and private subnets; added `external_vpc_subnet_ids` local that maps concatenated subnets to an "ExternalVpcSubnetIds" key; updated parameter merge to pass `external_vpc_subnet_ids` instead of `subnet_ids`

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

feat: support network_stack parameter #19: Related modification to subnet ID handling and external networking parameter structure.

Suggested labels

major

Suggested reviewers

Benbentwo
milldr

Poem

🐰 Two subnets emerge where one stood before,
Public paths and private doors,
Concatenated flows through new parameter gates—
Infrastructure splits, coordination awaits! 🌐

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "feat: separate external public/private subnet ids" directly and accurately describes the primary change in the changeset. The modifications across both `src/variables.tf` and `src/main.tf` involve refactoring the single `subnet_ids` parameter into two distinct parameters: `public_subnet_ids` and `private_subnet_ids`. The title is specific, concise (49 characters, 6 words), and clearly communicates the core intent without vague terminology or noise. It aligns with the PR objectives and enables reviewers to quickly understand the main change.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

mergify · 2025-10-24T08:51:21Z

Important

Do not edit the README.md directly. It's auto-generated from the README.yaml

Please update the README.yaml file instead.

Could you fix it @jochem725? 🙏

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/main.tf (1)

74-75: Fix typo in comment.

Line 74 contains a typo: "batties included" should likely be "batteries included" or possibly another phrase. Please correct.
src/variables.tf (1)
59-71: Enhance variable descriptions with context about networking_stack dependency.

The variable descriptions are minimal and don't explain the relationship to networking_stack or when these variables should be used. Consider enriching the descriptions to clarify:

These are only applicable when networking_stack = "external"

Whether one, both, or neither can be specified

The purpose of splitting into public vs. private (allows spawning workers in different subnet types)

Example enhancement:
variable "public_subnet_ids" {
  type        = list(string)
  description = "Public subnet IDs for external VPC networking stack. Used when networking_stack is set to 'external' to spawn public workers. Leave null when using embedded networking."
  nullable    = true
  default     = null
}

variable "private_subnet_ids" {
  type        = list(string)
  description = "Private subnet IDs for external VPC networking stack. Used when networking_stack is set to 'external' to spawn private workers. Leave null when using embedded networking."
  nullable    = true
  default     = null
}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dca5aa5 and e985555.

📒 Files selected for processing (2)

src/main.tf (3 hunks)
src/variables.tf (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Summary

🔇 Additional comments (2)

src/main.tf (1)

6-7: Verify subnet concatenation order aligns with runs-on expectations.

The code concatenates public subnets first, then private subnets. Confirm this ordering is intentional and expected by the runs-on template, particularly if the template has specific expectations about subnet positioning or purpose.

src/variables.tf (1)

59-71: Verify validation strategy for external networking requirements.

The PR objectives mention allowing users to spawn both public and private workers, but there's currently no validation to:

Require at least one subnet when networking_stack = "external"

Prevent users from specifying subnets when using networking_stack = "embedded" (though harmless, it could be confusing)

Confirm whether validation should be added to catch configuration errors earlier (at Terraform plan time) rather than at CloudFormation deployment time.

goruha

Pls make these changes to simplify the logic.

goruha · 2025-11-04T18:27:21Z

src/variables.tf

-  description = "Subnet IDs"
+  description = "Public subnet IDs"
+  nullable    = true
+  default     = null


Suggested change

default = null

default = []

goruha · 2025-11-04T18:27:32Z

src/variables.tf

+  type        = list(string)
+  description = "Private subnet IDs"
  nullable    = true
  default     = null


Suggested change

default = null

default = []

goruha · 2025-11-04T18:27:38Z

src/variables.tf

+variable "private_subnet_ids" {
+  type        = list(string)
+  description = "Private subnet IDs"
  nullable    = true


Suggested change

nullable = true

nullable = false

goruha · 2025-11-04T18:27:46Z

src/variables.tf

  type        = list(string)
-  description = "Subnet IDs"
+  description = "Public subnet IDs"
+  nullable    = true


Suggested change

nullable = true

nullable = false

goruha · 2025-11-04T18:28:15Z

src/main.tf

-  subnet_ids       = var.subnet_ids != null ? { "ExternalVpcSubnetIds" = join(",", var.subnet_ids) } : {}
+  external_vpc_id         = var.vpc_id != null ? { "ExternalVpcId" = var.vpc_id } : {}
+  networking_stack        = var.networking_stack != null ? { "NetworkingStack" = var.networking_stack } : {}
+  subnet_ids              = concat(coalesce(var.public_subnet_ids, []), coalesce(var.private_subnet_ids, []))


Suggested change

subnet_ids = concat(coalesce(var.public_subnet_ids, []), coalesce(var.private_subnet_ids, []))

subnet_ids = concat(var.public_subnet_ids, var.private_subnet_ids)

mergify bot requested review from a team October 24, 2025 08:51

mergify bot added the triage Needs triage label Oct 24, 2025

feat: separate external public/private subnet ids

e985555

jochem725 force-pushed the feature/allow-specifying-external-public-private-subnet-ids branch from dca5aa5 to e985555 Compare October 24, 2025 09:06

mergify bot added the needs-test Needs testing label Oct 24, 2025

coderabbitai bot reviewed Oct 24, 2025

View reviewed changes

goruha requested changes Nov 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat: separate external public/private subnet ids #43

feat: separate external public/private subnet ids #43

Uh oh!

jochem725 commented Oct 24, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 24, 2025 •

edited

Loading

Uh oh!

mergify bot commented Oct 24, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

goruha left a comment

Uh oh!

goruha Nov 4, 2025

Uh oh!

goruha Nov 4, 2025

Uh oh!

goruha Nov 4, 2025

Uh oh!

goruha Nov 4, 2025

Uh oh!

goruha Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	subnet_ids = concat(coalesce(var.public_subnet_ids, []), coalesce(var.private_subnet_ids, []))
	subnet_ids = concat(var.public_subnet_ids, var.private_subnet_ids)

Uh oh!

feat: separate external public/private subnet ids #43

Are you sure you want to change the base?

feat: separate external public/private subnet ids #43

Uh oh!

Conversation

jochem725 commented Oct 24, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

what

why

references

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

mergify bot commented Oct 24, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

goruha left a comment

Choose a reason for hiding this comment

Uh oh!

goruha Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

goruha Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

goruha Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

goruha Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

goruha Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jochem725 commented Oct 24, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 24, 2025 •

edited

Loading