fix(reserved IP): add feedback from PR#71 review

kral2 · kral2 · commit 473e1e50405f · 2021-09-20T14:40:12.000+02:00
- fix assign_public_ip test condition
- rename terraform resources from &lt;this&gt; to more meaningful name
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
@@ -26,8 +26,8 @@ Given a version number MAJOR.MINOR.PATCH:
 * Add support for freeform and defined tags for instances, vnics and block volumes (Fix #10, #11, #12, #13, #18, #20)
 * Add "module watermark" freeform tags: module defined and user defined freeform tags are merged on the final resource
 * Add support to provide the `ssh_authorized_keys` argument as a string or as a file (Fix #67 #70)
-* Provision instances with reserved Public IP
-* [ ] Define a backup policy for boot volume and additional block volumes
+* Add support for reserved Public IP on instance first VNIC (fix #55)
+* [ ] Define a backup policy for boot volume and additional block volumes (fix #64)
 * Add new outputs for each provisioned resources: "all_attributes" outputs have full provider coverage and are auto-updating.
 
 === Documentation
diff --git a/docs/terraformoptions.adoc b/docs/terraformoptions.adoc
@@ -24,15 +24,15 @@ No modules.
 [cols="a,a",options="header,autowidth"]
 |===
 |Name |Type
-|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/resources/core_instance[oci_core_instance.this] |resource
-|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/resources/core_public_ip[oci_core_public_ip.this] |resource
-|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/resources/core_volume[oci_core_volume.this] |resource
-|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/resources/core_volume_attachment[oci_core_volume_attachment.this] |resource
-|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/data-sources/core_instance_credentials[oci_core_instance_credentials.this] |data source
-|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/data-sources/core_private_ips[oci_core_private_ips.this] |data source
+|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/resources/core_instance[oci_core_instance.instance] |resource
+|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/resources/core_public_ip[oci_core_public_ip.public_ip] |resource
+|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/resources/core_volume[oci_core_volume.volume] |resource
+|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/resources/core_volume_attachment[oci_core_volume_attachment.volume_attachment] |resource
+|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/data-sources/core_instance_credentials[oci_core_instance_credentials.crendential] |data source
+|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/data-sources/core_private_ips[oci_core_private_ips.private_ips] |data source
 |https://registry.terraform.io/providers/hashicorp/oci/latest/docs/data-sources/core_shapes[oci_core_shapes.ad1] |data source
-|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/data-sources/core_subnet[oci_core_subnet.this] |data source
-|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/data-sources/core_vnic_attachments[oci_core_vnic_attachments.this] |data source
+|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/data-sources/core_subnet[oci_core_subnet.instance_subnet] |data source
+|https://registry.terraform.io/providers/hashicorp/oci/latest/docs/data-sources/core_vnic_attachments[oci_core_vnic_attachments.vnic_attachment] |data source
 |https://registry.terraform.io/providers/hashicorp/oci/latest/docs/data-sources/identity_availability_domains[oci_identity_availability_domains.ad] |data source
 |===
 
@@ -150,7 +150,7 @@ No modules.
 |no
 
 |[[input_public_ip]] <<input_public_ip,public_ip>>
-|OCID of the Public IP to attach to primary vnic. Valid values are NONE, RESERVED or EPHEMERAL.
+|Whether to create a Public IP to attach to primary vnic and wwhich lifetime. Valid values are NONE, RESERVED or EPHEMERAL.
 |`string`
 |`"NONE"`
 |no
diff --git a/examples/instances_fixed_shape/main.tf b/examples/instances_fixed_shape/main.tf
@@ -34,8 +34,8 @@ module "instance_nonflex" {
   # operating system parameters
   ssh_public_keys = var.ssh_public_keys
   # networking parameters
-  assign_public_ip = var.assign_public_ip
-  subnet_ocids     = var.subnet_ocids
+  public_ip    = var.public_ip # NONE, RESERVED or EPHEMERAL
+  subnet_ocids = var.subnet_ocids
   # storage parameters
   block_storage_sizes_in_gbs = var.block_storage_sizes_in_gbs
 }
@@ -65,8 +65,8 @@ module "instance_nonflex_custom" {
   # operating system parameters
   ssh_public_keys = var.ssh_public_keys
   # networking parameters
-  assign_public_ip = var.assign_public_ip
-  subnet_ocids     = var.subnet_ocids
+  public_ip    = var.public_ip # NONE, RESERVED or EPHEMERAL
+  subnet_ocids = var.subnet_ocids
   # storage parameters
   block_storage_sizes_in_gbs = [] # no block volume will be created
 }
diff --git a/examples/instances_fixed_shape/variables.tf b/examples/instances_fixed_shape/variables.tf
@@ -118,6 +118,12 @@ variable "assign_public_ip" {
   default     = false
 }
 
+variable "public_ip" {
+  description = "Whether to create a Public IP to attach to primary vnic and wwhich lifetime. Valid values are NONE, RESERVED or EPHEMERAL."
+  type        = string
+  default     = "NONE"
+}
+
 variable "subnet_ocids" {
   description = "The unique identifiers (OCIDs) of the subnets in which the instance primary VNICs are created."
   type        = list(string)
diff --git a/examples/instances_flex_shape/main.tf b/examples/instances_flex_shape/main.tf
@@ -37,8 +37,8 @@ module "instance_flex" {
   # operating system parameters
   ssh_public_keys = var.ssh_public_keys
   # networking parameters
-  assign_public_ip = var.assign_public_ip
-  subnet_ocids     = var.subnet_ocids
+  public_ip    = var.public_ip # NONE, RESERVED or EPHEMERAL
+  subnet_ocids = var.subnet_ocids
   # storage parameters
   block_storage_sizes_in_gbs = var.block_storage_sizes_in_gbs
 }
@@ -67,7 +67,7 @@ output "instance_flex" {
 #   # operating system parameters
 #   ssh_public_key = var.ssh_public_key
 #   # networking parameters
-#   assign_public_ip = var.assign_public_ip
+#   public_ip    = var.public_ip # NONE, RESERVED or EPHEMERAL
 #   subnet_ocids     = var.subnet_ocids
 #   # storage parameters
 #   block_storage_sizes_in_gbs = [] # no block volume will be created
diff --git a/examples/instances_flex_shape/variables.tf b/examples/instances_flex_shape/variables.tf
@@ -119,10 +119,10 @@ variable "ssh_public_keys" {
 
 # networking parameters
 
-variable "assign_public_ip" {
-  description = "Whether the VNIC should be assigned a public IP address."
-  type        = bool
-  default     = false
+variable "public_ip" {
+  description = "Whether to create a Public IP to attach to primary vnic and wwhich lifetime. Valid values are NONE, RESERVED or EPHEMERAL."
+  type        = string
+  default     = "NONE"
 }
 
 variable "subnet_ocids" {
diff --git a/examples/instances_reserved_public_ip/main.tf b/examples/instances_reserved_public_ip/main.tf
@@ -19,8 +19,7 @@ provider "oci" {
 
 # # * This module will create 1 Flex Compute Instances, with a reserved public IP
 module "instance_reserved_ip" {
-  source = "../../"
-  # source = "oracle-terraform-modules/compute-instance/oci"
+  source = "oracle-terraform-modules/compute-instance/oci"
   # general oci parameters
   compartment_ocid = var.compartment_ocid
   freeform_tags    = var.freeform_tags
diff --git a/examples/instances_reserved_public_ip/variables.tf b/examples/instances_reserved_public_ip/variables.tf
@@ -112,7 +112,7 @@ variable "ssh_authorized_keys" {
 # networking parameters
 
 variable "public_ip" {
-  description = "OCID of the Public IP to attach to primary vnic."
+  description = "Whether to create a Public IP to attach to primary vnic and wwhich lifetime. Valid values are NONE, RESERVED or EPHEMERAL."
   type        = string
   default     = "NONE"
 }
diff --git a/main.tf b/main.tf
@@ -24,7 +24,7 @@ locals {
 ####################
 # Subnet Datasource
 ####################
-data "oci_core_subnet" "this" {
+data "oci_core_subnet" "instance_subnet" {
   count     = length(var.subnet_ocids)
   subnet_id = element(var.subnet_ocids, count.index)
 }
@@ -56,7 +56,7 @@ locals {
 ############
 # Instance
 ############
-resource "oci_core_instance" "this" {
+resource "oci_core_instance" "instance" {
   count = var.instance_count
   // If no explicit AD number, spread instances on all ADs in round-robin. Looping to the first when last AD is reached
   availability_domain  = var.ad_number == null ? element(local.ADs, count.index) : element(local.ADs, var.ad_number - 1)
@@ -74,7 +74,7 @@ resource "oci_core_instance" "this" {
   }
 
   create_vnic_details {
-    assign_public_ip = var.public_ip == null ? var.assign_public_ip : false
+    assign_public_ip = var.public_ip == "NONE" ? var.assign_public_ip : false
     display_name     = var.vnic_name == "" ? "" : var.instance_count != "1" ? "${var.vnic_name}_${count.index + 1}" : var.vnic_name
     hostname_label   = var.hostname_label == "" ? "" : var.instance_count != "1" ? "${var.hostname_label}-${count.index + 1}" : var.hostname_label
     private_ip = element(
@@ -83,7 +83,7 @@ resource "oci_core_instance" "this" {
     )
     skip_source_dest_check = var.skip_source_dest_check
     // Current implementation requires providing a list of subnets when using ad-specific subnets
-    subnet_id = data.oci_core_subnet.this[count.index % length(data.oci_core_subnet.this.*.id)].id
+    subnet_id = data.oci_core_subnet.instance_subnet[count.index % length(data.oci_core_subnet.instance_subnet.*.id)].id
 
     freeform_tags = local.merged_freeform_tags
     defined_tags  = var.defined_tags
@@ -111,19 +111,19 @@ resource "oci_core_instance" "this" {
 ##################################
 # Instance Credentials Datasource
 ##################################
-data "oci_core_instance_credentials" "this" {
+data "oci_core_instance_credentials" "crendential" {
   count       = var.resource_platform != "linux" ? var.instance_count : 0
-  instance_id = oci_core_instance.this[count.index].id
+  instance_id = oci_core_instance.instance[count.index].id
 }
 
 #########
 # Volume
 #########
-resource "oci_core_volume" "this" {
+resource "oci_core_volume" "volume" {
   count               = var.instance_count * length(var.block_storage_sizes_in_gbs)
-  availability_domain = oci_core_instance.this[count.index % var.instance_count].availability_domain
+  availability_domain = oci_core_instance.instance[count.index % var.instance_count].availability_domain
   compartment_id      = var.compartment_ocid
-  display_name        = "${oci_core_instance.this[count.index % var.instance_count].display_name}_volume${floor(count.index / var.instance_count)}"
+  display_name        = "${oci_core_instance.instance[count.index % var.instance_count].display_name}_volume${floor(count.index / var.instance_count)}"
   size_in_gbs = element(
     var.block_storage_sizes_in_gbs,
     floor(count.index / var.instance_count),
@@ -135,44 +135,44 @@ resource "oci_core_volume" "this" {
 ####################
 # Volume Attachment
 ####################
-resource "oci_core_volume_attachment" "this" {
+resource "oci_core_volume_attachment" "volume_attachment" {
   count           = var.instance_count * length(var.block_storage_sizes_in_gbs)
   attachment_type = var.attachment_type
-  instance_id     = oci_core_instance.this[count.index % var.instance_count].id
-  volume_id       = oci_core_volume.this[count.index].id
+  instance_id     = oci_core_instance.instance[count.index % var.instance_count].id
+  volume_id       = oci_core_volume.volume[count.index].id
   use_chap        = var.use_chap
 }
 
 ####################
 # Networking
 ####################
 
-data "oci_core_vnic_attachments" "this" {
+data "oci_core_vnic_attachments" "vnic_attachment" {
   count          = var.instance_count
   compartment_id = var.compartment_ocid
-  instance_id    = oci_core_instance.this[count.index].id
+  instance_id    = oci_core_instance.instance[count.index].id
 
   depends_on = [
-    oci_core_instance.this
+    oci_core_instance.instance
   ]
 }
 
-data "oci_core_private_ips" "this" {
+data "oci_core_private_ips" "private_ips" {
   count   = var.instance_count
-  vnic_id = data.oci_core_vnic_attachments.this[count.index].vnic_attachments[0].vnic_id
+  vnic_id = data.oci_core_vnic_attachments.vnic_attachment[count.index].vnic_attachments[0].vnic_id
 
   depends_on = [
-    oci_core_instance.this
+    oci_core_instance.instance
   ]
 }
 
-resource "oci_core_public_ip" "this" {
+resource "oci_core_public_ip" "public_ip" {
   count          = var.public_ip == "NONE" ? 0 : var.instance_count
   compartment_id = var.compartment_ocid
   lifetime       = var.public_ip
 
-  display_name  = var.public_ip_display_name != null ? var.public_ip_display_name : oci_core_instance.this[count.index].display_name
-  private_ip_id = data.oci_core_private_ips.this[count.index].private_ips[0].id
+  display_name  = var.public_ip_display_name != null ? var.public_ip_display_name : oci_core_instance.instance[count.index].display_name
+  private_ip_id = data.oci_core_private_ips.private_ips[count.index].private_ips[0].id
   # public_ip_pool_id = oci_core_public_ip_pool.test_public_ip_pool.id # * (BYOIP CIDR Blocks) are not supported yet by this module.
 
   freeform_tags = local.merged_freeform_tags
diff --git a/outputs.tf b/outputs.tf
@@ -3,7 +3,7 @@
 locals {
   instances_details = [
     // display name, Primary VNIC Public/Private IP for each instance
-    for i in oci_core_instance.this : <<EOT
+    for i in oci_core_instance.instance : <<EOT
     ${~i.display_name~}
     Primary-PublicIP: %{if i.public_ip != ""}${i.public_ip~}%{else}N/A%{endif~}
     Primary-PrivateIP: ${i.private_ip~}
@@ -18,59 +18,59 @@ output "instances_summary" {
 
 output "instance_id" {
   description = "ocid of created instances. "
-  value       = oci_core_instance.this.*.id
+  value       = oci_core_instance.instance.*.id
 }
 
 output "private_ip" {
   description = "Private IPs of created instances. "
-  value       = oci_core_instance.this.*.private_ip
+  value       = oci_core_instance.instance.*.private_ip
 }
 
 output "public_ip" {
   description = "Public IPs of created instances. "
-  value       = oci_core_instance.this.*.public_ip
+  value       = oci_core_instance.instance.*.public_ip
 }
 
 output "instance_username" {
   description = "Usernames to login to Windows instance. "
-  value       = data.oci_core_instance_credentials.this.*.username
+  value       = data.oci_core_instance_credentials.crendential.*.username
 }
 
 output "instance_password" {
   description = "Passwords to login to Windows instance. "
   sensitive   = true
-  value       = data.oci_core_instance_credentials.this.*.password
+  value       = data.oci_core_instance_credentials.crendential.*.password
 }
 
 # New complete outputs for each resources with provider parity. Auto-updating.
 # Usefull for module composition.
 
 output "instance_all_attributes" {
   description = "all attributes of created instance"
-  value       = { for k, v in oci_core_instance.this : k => v }
+  value       = { for k, v in oci_core_instance.instance : k => v }
 }
 
 output "public_ip_all_attributes" {
   description = "all attributes of created public ip"
-  value       = { for k, v in oci_core_public_ip.this : k => v }
+  value       = { for k, v in oci_core_public_ip.public_ip : k => v }
 }
 
 output "private_ips_all_attributes" {
   description = "all attributes of created private ips"
-  value       = { for k, v in data.oci_core_private_ips.this : k => v }
+  value       = { for k, v in data.oci_core_private_ips.private_ips : k => v }
 }
 
 output "vnic_attachment_all_attributes" {
   description = "all attributes of created vnic attachments"
-  value       = { for k, v in data.oci_core_vnic_attachments.this : k => v }
+  value       = { for k, v in data.oci_core_vnic_attachments.vnic_attachment : k => v }
 }
 
 output "volume_all_attributes" {
   description = "all attributes of created volumes"
-  value       = { for k, v in oci_core_volume.this : k => v }
+  value       = { for k, v in oci_core_volume.volume : k => v }
 }
 
 output "volume_attachment_all_attributes" {
   description = "all attributes of created volumes attachments"
-  value       = { for k, v in oci_core_volume_attachment.this : k => v }
+  value       = { for k, v in oci_core_volume_attachment.volume_attachment : k => v }
 }
diff --git a/variables.tf b/variables.tf
@@ -151,7 +151,7 @@ variable "private_ips" {
 }
 
 variable "public_ip" {
-  description = "OCID of the Public IP to attach to primary vnic. Valid values are NONE, RESERVED or EPHEMERAL."
+  description = "Whether to create a Public IP to attach to primary vnic and wwhich lifetime. Valid values are NONE, RESERVED or EPHEMERAL."
   type        = string
   default     = "NONE"
 

Original file line number	Diff line number	Diff line change
`@@ -112,7 +112,7 @@ variable "ssh_authorized_keys" {`
`112`	`112`	`# networking parameters`
`113`	`113`
`114`	`114`	`variable "public_ip" {`
`115`		`- description = "OCID of the Public IP to attach to primary vnic."`
	`115`	`+ description = "Whether to create a Public IP to attach to primary vnic and wwhich lifetime. Valid values are NONE, RESERVED or EPHEMERAL."`
`116`	`116`	`type = string`
`117`	`117`	`default = "NONE"`
`118`	`118`	`}`
Original file line number	Diff line number	Diff line change
`@@ -151,7 +151,7 @@ variable "private_ips" {`
`151`	`151`	`}`
`152`	`152`
`153`	`153`	`variable "public_ip" {`
`154`		`- description = "OCID of the Public IP to attach to primary vnic. Valid values are NONE, RESERVED or EPHEMERAL."`
	`154`	`+ description = "Whether to create a Public IP to attach to primary vnic and wwhich lifetime. Valid values are NONE, RESERVED or EPHEMERAL."`
`155`	`155`	`type = string`
`156`	`156`	`default = "NONE"`
`157`	`157`