feat: separate external public/private subnet ids

[jochem725]

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]

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[mergify]

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:

1. These are only applicable when networking_stack = "external"
2. Whether one, both, or neither can be specified
3. 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:

1. Require at least one subnet when networking_stack = "external"
2. 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.