Skip to content

Conversation

@tpdownes
Copy link
Contributor

@tpdownes tpdownes commented Jun 2, 2022

  • if public IPs are enabled (default), the output is the list of external "NAT" IPs for each VM
  • if public IPs are disabled explicitly, the output is the empty list

We already have a task to expand test coverage of this module and I will add public/private IP to its description.

A blueprint that one can use to test this is below. The terraform outputs will yield:

external_ip_private = []
external_ip_public = [
  "104.197.82.46",
  "35.223.38.149",
  "34.71.34.33",
  "34.72.25.136",
  "34.123.15.216",
]
internal_ip_private = [
  "10.0.0.9",
  "10.0.0.3",
  "10.0.0.8",
]
internal_ip_public = [
  "10.0.0.2",
  "10.0.0.4",
  "10.0.0.7",
  "10.0.0.5",
  "10.0.0.6",
]
---
blueprint_name: vm-test

vars:
  project_id:  ## Set GCP Project ID Here ##
  deployment_name: vm-test-001
  region: us-central1
  zone: us-central1-c


deployment_groups:
- group: vm_test
  modules:
  - source: modules/network/vpc
    kind: terraform
    id: network1
  - source: modules/compute/vm-instance
    kind: terraform
    id: private
    use:
    - network1
    settings:
      instance_count: 3
      name_prefix: private
      disable_public_ips: true
    outputs:
    - external_ip
    - internal_ip
  - source: modules/compute/vm-instance
    kind: terraform
    id: public
    use:
    - network1
    settings:
      instance_count: 5
      name_prefix: public
    outputs:
    - external_ip
    - internal_ip

Submission Checklist

  • Have you installed and run this change against pre-commit? (pre-commit install)
  • Are all tests passing? (make tests)
  • Have you written unit tests to cover this change?
  • Is unit test coverage still above 80%?
  • Have you updated all applicable documentation?
  • Have you followed the guidelines in our Contributing document?

- if public IPs are enabled (default), the output is the list of
  external "NAT" IPs for each VM
- if public IPs are disabled explicitly, the output is the empty list
Copy link
Contributor

@heyealex heyealex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Only comment I considered adding was updating this and internal_ip to be plural since they output lists. That should probably be done separately though.

@tpdownes
Copy link
Contributor Author

tpdownes commented Jun 2, 2022

Yeah, I have a task that describes the plural/singular issue but consider that a specific change with potential breaking implications.

@tpdownes tpdownes merged commit 6fe345f into GoogleCloudPlatform:develop Jun 2, 2022
@tpdownes tpdownes deleted the vm_instance_external_ip branch June 2, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants