feat: Port to Etree Network connection examples#212
Conversation
| client_secret = var.equinix_client_secret | ||
| } | ||
|
|
||
| module "create_port_2_network_connection" { |
There was a problem hiding this comment.
We need to toss an ETree network resource into this example also. As it is there is very little value add here. Please update the README.md to reflect that change as well.
| network { | ||
| uuid = var.zside_network_uuid | ||
| } | ||
| role = contains(["EVPTREE_VC", "EPTREE_VC"], var.connection_type) && var.role != "" ? var.role : null |
There was a problem hiding this comment.
Are there only 2 role types? Could this be checked at the provider level so that we don't have to update modules if the enum changes?
If we can do this at provider level then remove the enum check and allow whatever value they want to be put in here because the provider or API error will give them enough detail.
There was a problem hiding this comment.
Actually, I see that you're just checking the connection type here. That's a good double check, but this can still be at the provider level.
There was a problem hiding this comment.
In retrospect, I think I could also skip this check, as the API also gives clear error messages in 400 bad request. Will remove this check.
| network { | ||
| uuid = var.zside_network_uuid | ||
| } | ||
| role = var.role |
There was a problem hiding this comment.
You also don't have any check on the secondary connection so it will put an empty string here if it's not provided. Add the same check on it that makes it null if not provided.
There was a problem hiding this comment.
We need the attention to detail in our code to be consistent across the edge cases.
| required_version = ">= 1.5.4" | ||
| required_providers { | ||
| equinix = { | ||
| source="equinix/equinix" |
There was a problem hiding this comment.
nit: formatting is wrong on the source line
| aside_vlan_tag = var.aside_vlan_tag | ||
|
|
||
| # Z-side | ||
| zside_service_token_uuid = equinix_fabric_service_token.service-token.id |
There was a problem hiding this comment.
Do we not need the role for a zside service token on etree network?
There was a problem hiding this comment.
No, it's not needed.
thogarty
left a comment
There was a problem hiding this comment.
More changes requested
| variable "network_scope" { | ||
| description = "Equinix Network Scope" | ||
| type = string | ||
| } No newline at end of file |
| term_length = 12 | ||
| aside_port_name = "ops-user100-CX-SV5-NL-Qinq-STD-1G-SEC-JP-190" | ||
| aside_vlan_tag = "1976" | ||
| zside_service_token_uuid = "Service_Token_UUID" |
There was a problem hiding this comment.
I counted 26 defined variables in variables.tf but there are 28 in the example. What are the mismatches? Has this been tested with no variable warnings?
There was a problem hiding this comment.
I updated here. But the numbers in variables.tf and example do not match because there are some hardcoded values in main.tf , such as service token type - "VC_TOKEN" and allow_remote_connection = true
| variable "role" { | ||
| description = "Role of network" | ||
| type = string | ||
| } |
There was a problem hiding this comment.
There's a mismatch between defined variables and example values in this example here. Let's remove all bloat. Keep it minimal without any variable warnings.
There was a problem hiding this comment.
notification_type and notification_emails is common field for both etree_network resource and the module create_port_2_network_connection so the numbers don't match in variables.tf and example file.
| default = [] | ||
| } | ||
| variable "role" { | ||
| description = "Role of network" |
There was a problem hiding this comment.
This is only the role of the ETree Networks. So we need to be specific.
thogarty
left a comment
There was a problem hiding this comment.
More additional comments.
| @@ -0,0 +1,13 @@ | |||
| output "service_token_id" { | |||
There was a problem hiding this comment.
We could output the network here too.
| @@ -0,0 +1,9 @@ | |||
| output "port_connection" { | |||
There was a problem hiding this comment.
Once we add the network we want to add it to the output also.
|
This PR is included in version 0.25.0 🎉 |
Uh oh!
There was an error while loading. Please reload this page.