feat: enhance naming for vpc resources and fix prefix checks #685

rajatagarwal-ibm · 2023-12-04T12:33:26Z

Description

Addresses:

Release required?

No release
Patch release (x.x.X)
Minor release (x.X.x)
Major release (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:

/run pipeline

Checklist for reviewers

If relevant, a test for the change is included or updated with this PR.
If relevant, documentation for the change is included or updated with this PR.

For mergers

Use a conventional commit message to set the release level. Follow the guidelines.
Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
Use the Squash and merge option.

rajatagarwal-ibm · 2023-12-04T18:46:57Z

While the code says public gateway creation is optional (

terraform-ibm-landing-zone-vpc/main.tf

Line 218 in f03b300

# Public Gateways (Optional)

) but in reality I have found that those are always created. For example, I need to add the public_gateway_name variable to make an "existing_vpc" example.

If we want to keep the public gateway optional - perhaps we need to create a flag or something else in the logic

rajatagarwal-ibm · 2023-12-04T18:48:13Z

Ran every test locally and also ran hub-spoke-manual-resolver apply (doesn't have test) - everything worked fine.

SKIP UPGRADE TEST: change of vpc name in main.tf

rajatagarwal-ibm · 2023-12-04T19:01:48Z

~~Need to skip the upgrade test as there is a change of the vpc name in the main.tf (consumers won't be impacted as var.name is unchanged.)~~

Not skipping upgrade test anymore as reverted changes back on the vpc name with prefix.

ocofaigh · 2023-12-05T18:03:42Z

@rajatagarwal-ibm It seems you added several things since skipping the upgrade test so I'm not comfortable with it being skipped. Can you unskip it (use commit with UNSKIP UPGRADE TEST) and paste the results. We can then make a call on skipping it if no breaking changes are identified.

UNSKIP UPGRADE TEST: testing

UNSKIP UPGRADE TEST: removed variable name change

rajatagarwal-ibm · 2023-12-06T10:05:42Z

@ocofaigh

The upgrade test fails only because of the change in the VPC name, here:
name = var.prefix != null ? "${var.prefix}-vpc" : var.name

Local run test result:

TestRunUpgradeDefaultExample 2023-12-06T09:51:28Z command.go:185:   # module.slz_vpc.ibm_is_vpc.vpc[0] will be updated in-place
TestRunUpgradeDefaultExample 2023-12-06T09:51:28Z command.go:185:   ~ resource "ibm_is_vpc" "vpc" {
TestRunUpgradeDefaultExample 2023-12-06T09:51:28Z command.go:185:         id                          = "r042-e8e37261-70f4-4951-b2ea-a48baf273fd3"
TestRunUpgradeDefaultExample 2023-12-06T09:51:28Z command.go:185:       ~ name                        = "slz-vpc-upg-hki9xw-vpc-vpc" -> "slz-vpc-upg-hki9xw-vpc"
TestRunUpgradeDefaultExample 2023-12-06T09:51:28Z command.go:185:         tags                        = []
TestRunUpgradeDefaultExample 2023-12-06T09:51:28Z command.go:185:         # (25 unchanged attributes hidden)
TestRunUpgradeDefaultExample 2023-12-06T09:51:28Z command.go:185: 
TestRunUpgradeDefaultExample 2023-12-06T09:51:28Z command.go:185:         # (1 unchanged block hidden)
TestRunUpgradeDefaultExample 2023-12-06T09:51:28Z command.go:185:     }

I didn't like the naming this way so removed one variable, but I guess I will revert it back as its not required as part of this issue.

ocofaigh

@rajatagarwal-ibm See my comments please

tests/pr_test.go

main.tf

ocofaigh · 2023-12-08T14:01:54Z

examples/existing_vpc/variables.tf

+variable "public_gateway_name" {
+  description = "The name of the public gateway"
+  type        = string
+}


Instead of adding a new variable, can you just hard code the variable values in the main.tf of the existing_vpc example main.tf ?

examples/existing_vpc/main.tf

ocofaigh · 2023-12-08T14:10:35Z

variables.tf

@@ -3,11 +3,47 @@
 ##############################################################################

 variable "name" {
-  description = "The name to give the newly provisioned VPC. Only used if 'create_vpc' is true."
+  description = "The name to give the provisioned VPC. Only used if 'create_vpc' is true."


hmm, name is also used for other resources, not just the VPC. I would update this to:

"The string to use for the naming of VPC resources."

I think you should also then add a new variable called vpc_name which can be used to explicitly name the VPC.

This module is being consumed in the SLZ module. Any changes which we make here with name will impact that too. I don't think so that's needed. Hence my proposal is I can rename the description to the following:

"The string to use for the naming of VPC, when var.create_vpc is true. This string also use for the naming of VPC resources."

ocofaigh · 2023-12-08T14:24:34Z

main.tf

@@ -193,7 +198,7 @@ resource "time_sleep" "wait_for_authorization_policy" {

 resource "ibm_is_vpc_routing_table" "route_table" {
  for_each                      = module.dynamic_values.routing_table_map
-  name                          = var.prefix != null ? "${var.prefix}-${var.name}-route-${each.value.name}" : "${var.name}-route-${each.value.name}"
+  name                          = var.prefix != null ? "${var.prefix}-${var.name}-route-${each.value.name}" : "${var.routing_table_name}-${each.value.name}"


I don't see any reason to add var.routing_table_name - you can achieve this naming with the current code using var.name. There can be multiple routing tables here, so we will always be appending ${each.value.name} to the name here

As the objective of the task is to give freedom to the user to name it anything they like, I have added these as variables. If they don't provide the value then it will pick up either prefix or name.

But how are you going to handle the naming of multiple routing tables with 1 string variable. You dont want them all named the same, so hence you will need to use ${each.value.name} in the naming

ocofaigh · 2023-12-08T14:25:16Z

main.tf

@@ -228,7 +233,7 @@ locals {

 resource "ibm_is_public_gateway" "gateway" {
  for_each       = local.gateway_object
-  name           = var.prefix != null ? "${var.prefix}-${var.name}-public-gateway-${each.key}" : "${var.name}-public-gateway-${each.key}"
+  name           = var.prefix != null ? "${var.prefix}-${var.name}-public-gateway-${each.key}" : "${var.public_gateway_name}-${each.key}"


I don't see any reason to add var.public_gateway_name - you can achieve this naming with the current code using var.name. There can be multiple public gateways here, so we will always be appending ${each.value.name} to the name here

As the objective of the task is to give freedom to the user to name it anything they like, I have added these as variables. If they don't provide the value then it will pick up either prefix or name.

same as #685 (comment)

variables.tf

…raform-ibm-modules/terraform-ibm-landing-zone-vpc into enhance-naming-and-fix-prefix-checks

vburckhardt

Hi Rajat, some comments

main.tf

rajatagarwal-ibm · 2023-12-19T10:54:00Z

/run pipeline

vburckhardt · 2023-12-19T10:58:17Z

/run pipeline

…raform-ibm-modules/terraform-ibm-landing-zone-vpc into enhance-naming-and-fix-prefix-checks

rajatagarwal-ibm · 2023-12-19T11:10:51Z

/run pipeline

rajatagarwal-ibm · 2023-12-19T11:48:09Z

/run pipeline

SKIP UPGRADE TEST: removed vpc from prefix name in gateway name

rajatagarwal-ibm · 2023-12-19T12:02:20Z

/run pipeline

ocofaigh

@rajatagarwal-ibm I left some comments - maybe we need to jump on a call to discuss how we want this to look as its starting to get confusing now to support all the use cases

dynamic_values.tf

main.tf

SKIP UPGRADE TEST: removed vpc from prefix name in gateway name

main.tf

variables.tf

README.md

ocofaigh · 2023-12-21T14:20:30Z

main.tf

  resource_group = var.resource_group_id
  classic_access = var.classic_access
  # address prefix is set to auto only if no address prefixes NOR any subnet is passed as input
-  address_prefix_management   = (length([for prefix in values(coalesce(var.address_prefixes, {})) : prefix if prefix != null]) != 0) || (length([for subnet in values(coalesce(var.subnets, {})) : subnet if subnet != null]) != 0) ? "manual" : null
+  address_prefix_management   = length([for prefix in values(coalesce(var.address_prefixes, {})) : prefix if prefix != null]) != 0 ? "manual" : null


why is this change needed?

variables.tf

SKIP UPGRADE TEST: removed vpc from prefix name in gateway name

rajatagarwal-ibm · 2023-12-22T13:57:37Z

Created #697 since we can no longer bypass the upgrade test in this PR

This can be closed

rajatagarwal-ibm added 2 commits December 4, 2023 12:31

feat: enhance naming for vpc resources and fix prefix checks

6142430

feat: enhance naming for vpc resources and fix prefix checks

07dfc7e

rajatagarwal-ibm added 2 commits December 4, 2023 18:58

feat: enhance naming for vpc resources and fix prefix checks

a89220e

feat: enhance naming for vpc resources and fix prefix checks

41ff7b5

SKIP UPGRADE TEST: change of vpc name in main.tf

rajatagarwal-ibm and others added 5 commits December 4, 2023 19:22

feat: enhance naming for vpc resources and fix prefix checks

badddf6

feat: enhance naming for vpc resources and fix prefix checks

0885812

feat: enhance naming for vpc resources and fix prefix checks

57a611b

feat: enhance naming for vpc resources and fix prefix checks

f371d81

Merge branch 'main' into enhance-naming-and-fix-prefix-checks

6093cf3

rajatagarwal-ibm added 2 commits December 6, 2023 09:48

feat: enhance naming for vpc resources and fix prefix checks

4f701f9

UNSKIP UPGRADE TEST: testing

feat: enhance naming for vpc resources and fix prefix checks

bd8815d

UNSKIP UPGRADE TEST: removed variable name change

rajatagarwal-ibm requested a review from vburckhardt December 7, 2023 16:52

ocofaigh requested changes Dec 8, 2023

View reviewed changes

rajatagarwal-ibm and others added 4 commits December 11, 2023 14:21

Merge branch 'main' into enhance-naming-and-fix-prefix-checks

5387335

feat: enhance naming for vpc resources and fix prefix checks

1b56664

Merge branch 'enhance-naming-and-fix-prefix-checks' of github.com:ter…

49bfcee

…raform-ibm-modules/terraform-ibm-landing-zone-vpc into enhance-naming-and-fix-prefix-checks

Merge branch 'main' into enhance-naming-and-fix-prefix-checks

1243226

rajatagarwal-ibm requested a review from ocofaigh December 14, 2023 11:43

Merge branch 'main' into enhance-naming-and-fix-prefix-checks

02c3166

vburckhardt reviewed Dec 18, 2023

View reviewed changes

main.tf Outdated Show resolved Hide resolved

main.tf Outdated Show resolved Hide resolved

rajatagarwal-ibm and others added 5 commits December 19, 2023 10:11

feat: enhance naming for vpc resources and fix prefix checks

79cef09

Merge branch 'main' into enhance-naming-and-fix-prefix-checks

d22b88f

feat: enhance naming for vpc resources and fix prefix checks

7137c3f

docs: improve descriptions

cf6526e

feat: enhance naming for vpc resources and fix prefix checks

9cdee99

feat: enhance naming for vpc resources and fix prefix checks

3f7e542

vburckhardt added 2 commits December 19, 2023 10:55

docs: improve descriptions

c436dd1

docs: improve descriptions

82731bc

rajatagarwal-ibm added 2 commits December 19, 2023 11:10

feat: enhance naming for vpc resources and fix prefix checks

4daea9e

Merge branch 'enhance-naming-and-fix-prefix-checks' of github.com:ter…

8ff34c1

…raform-ibm-modules/terraform-ibm-landing-zone-vpc into enhance-naming-and-fix-prefix-checks

feat: enhance naming for vpc resources and fix prefix checks

76dbcda

feat: enhance naming for vpc resources and fix prefix checks

25f69ba

SKIP UPGRADE TEST: removed vpc from prefix name in gateway name

ocofaigh requested changes Dec 19, 2023

View reviewed changes

feat: enhance naming for vpc resources and fix prefix checks

3b56215

SKIP UPGRADE TEST: removed vpc from prefix name in gateway name