-
Notifications
You must be signed in to change notification settings - Fork 265
modules/azure: Enable the use of external master & worker subnets #550
Conversation
Can one of the admins verify this patch? |
platforms/azure/variables.tf
Outdated
|
||
variable "tectonic_azure_external_master_subnet_id" { | ||
type = "string" | ||
description = "Subnet ID within an existing VNet to deploy master nodes into. Required to use an existing VNet. Example: the subnet ID starts with `\"/subscriptions/{subscriptionId}\"` or `\"/providers/{resourceProviderNamespace}\"'`. " |
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.
Suggestion: let's use HEREDOC syntax, so this does not end up in one long huge line, escaping of quotes is not necessary any more, and double new-lines will be translated to <br>
lines in the generated markdown:
description = <<EOF
Subnet ID within an existing VNet to deploy master nodes into.
Required to use an existing VNet.
Example: the subnet ID starts with `"/subscriptions/{subscriptionId}"` or `"/providers/{resourceProviderNamespace}"'`. "
EOF
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.
Additionally I suggest to add (optional) like so:
description = <<EOF
(optional) Subnet ID within an existing VNet to deploy master nodes into.
...
EOF
The terraform-examples tool will auto-detect the prefix (optional)
and will comment out the variable in the generated examples tfvars file.
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.
just two nits/suggestions regarding the documentation, else LGTM.
platforms/azure/variables.tf
Outdated
|
||
variable "tectonic_azure_external_master_subnet_id" { | ||
type = "string" | ||
description = "Subnet ID within an existing VNet to deploy master nodes into. Required to use an existing VNet. Example: the subnet ID starts with `\"/subscriptions/{subscriptionId}\"` or `\"/providers/{resourceProviderNamespace}\"'`. " |
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.
Additionally I suggest to add (optional) like so:
description = <<EOF
(optional) Subnet ID within an existing VNet to deploy master nodes into.
...
EOF
The terraform-examples tool will auto-detect the prefix (optional)
and will comment out the variable in the generated examples tfvars file.
@s-urbaniak Thanks for the feedback! I've gone ahead and addressed your changes |
f346739
to
813295d
Compare
ok to test |
1 similar comment
ok to test |
- Only create master & worker subnets if no external vnets exist - The `join()` interpolation function is used to work around hashicorp/hil#50 when the subnets are conditionally created. For more detail, see: coreos@7ab31b0)
@s-urbaniak is there anything else needed in this PR? |
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.
This LGTM now, thanks a lot!
No description provided.