-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for creating instances using the new VNI #737
Conversation
/run pipeline |
/run pipeline |
The VSIs will be re-created. |
/run pipeline |
/run pipeline |
/run pipeline |
main.tf
Outdated
# Create Virtual Network Interface | ||
############################################################################## | ||
resource "ibm_is_virtual_network_interface" "primary_vni" { | ||
for_each = !var.use_legacy_network_interface ? local.vsi_map : {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have had issues with this type of for_each in the past (using a conditional for a map). See line 96 of this file for a safer way to do this, or https://github.com/terraform-ibm-modules/terraform-ibm-landing-zone-vpc/blob/main/network_acls.tf#L163 to see how we discovered the issue with this pattern.
main.tf
Outdated
@@ -99,6 +162,13 @@ resource "ibm_is_subnet_reserved_ip" "vsi_ip" { | |||
auto_delete = false | |||
} | |||
|
|||
resource "ibm_is_subnet_reserved_ip" "secondary_vsi_ip" { | |||
for_each = var.number_of_secondary_reserved_ips > 0 && !var.use_legacy_network_interface ? local.secondary_reserved_ips_map : {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the for_each in the vsi_ip above for how to avoid a bug in terraform with a conditional returning map
} | ||
content { | ||
name = network_attachments.value.name | ||
virtual_network_interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all of the VNIs should be managed by terraform outside the VSI block, just like you have done with the primary. That will make it much easier to leave them alone and simply reattach if the VSI gets recreated, and they will retain all of their settings and IPs. Basically we want the VSI block here to manage as little as possible and we would "attach" resources managed outside the block instead. This way if a rollback occurs (or the VSI gets recreated for any reason) it will not destroy some of these other attachments (like VNIs) so that they maintain their state when reattached.
/run pipeline |
/run pipeline |
variables.tf
Outdated
@@ -116,6 +116,13 @@ variable "manage_reserved_ips" { | |||
default = false | |||
} | |||
|
|||
variable "number_of_secondary_reserved_ips" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried that at some point, someone is going to ask for this same feature for the secondary VNI, so we might need to change this input name. maybe instead of "secondary" something like "additional"? Also this setting only applies to the primary VNI, so that could be part of the name as well? then if we ever needed an entry here for the secondary VNI we could name it that way.
So perhaps something like primary_vni_additional_ip_count
? What do you think? Then later if someone really wants this same thing but for the secondary VNI, you could have a secondary_vni_
variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
/run pipeline |
/run pipeline |
🎉 This PR is included in version 4.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Release required?
x.x.X
)x.X.x
)X.x.x
)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers