-
Notifications
You must be signed in to change notification settings - Fork 2
Modified prefix/resources naming management #50
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
base: main
Are you sure you want to change the base?
Changes from all commits
f218189
8dd8957
a1e926a
65a8a62
26d3441
178fd25
76ac40b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,14 +10,32 @@ variable "ibmcloud_api_key" { | |
|
|
||
| variable "prefix" { | ||
| type = string | ||
| description = "Prefix for name of all resource created by this example" | ||
| nullable = true | ||
| description = "The prefix for the TFE instance name. The resources will be created starting from this, i.e. cluster will be named '[prefix_][instancename]_cluster'. Default set to empty string." | ||
| validation { | ||
| error_message = "Prefix must begin and end with a letter and contain only letters, numbers, and - characters." | ||
| condition = can(regex("^([A-z]|[a-z][-a-z0-9]*[a-z0-9])$", var.prefix)) | ||
| error_message = "var.prefix must begin and end with a letter and contain only letters, numbers, and - characters." | ||
| condition = (var.prefix == null || var.prefix == "" ? true : | ||
| alltrue([ | ||
| can(regex("^[a-z][-a-z0-9]*[a-z0-9]$", var.prefix)), | ||
| length(regexall("--", var.prefix)) == 0 | ||
| ]) | ||
| ) | ||
| } | ||
| default = "tfe" | ||
| default = "" | ||
| } | ||
|
|
||
| variable "instance_name" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't expose |
||
| type = string | ||
| nullable = false | ||
| description = "The TFE instance name. The resources will be created starting from this, i.e. cluster will be named '[instance_name]_cluster'. Default set to tfe_instance" | ||
| validation { | ||
| error_message = "var.instance_name must begin and end with a letter and contain only letters, numbers, and - characters." | ||
| condition = can(regex("^([A-z]|[a-z][-a-z0-9]*[a-z0-9])$", var.instance_name)) | ||
| } | ||
| default = "tfeinstance" | ||
| } | ||
|
|
||
|
|
||
| variable "region" { | ||
| type = string | ||
| description = "Region where resources are created" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ module "resource_group" { | |
| source = "terraform-ibm-modules/resource-group/ibm" | ||
| version = "1.4.0" | ||
| # if an existing resource group is not set (null) create a new one using prefix | ||
| resource_group_name = var.existing_resource_group_name == null ? "${var.prefix}-resource-group" : null | ||
| resource_group_name = var.existing_resource_group_name == null ? "${var.instance_name}-rg" : null | ||
| existing_resource_group_name = var.existing_resource_group_name | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. none of our modules support creating a resource group - this should be the same. It should take in a resource group ID. The DA should take in a resource group name (and use the resource group picker with default value of "Default") |
||
| } | ||
|
|
||
|
|
@@ -21,9 +21,9 @@ module "cos" { | |
| region = var.region | ||
| create_cos_instance = var.existing_cos_instance_id != null ? false : true | ||
| existing_cos_instance_id = var.existing_cos_instance_id | ||
| cos_instance_name = var.cos_instance_name != null ? var.cos_instance_name : "${var.prefix}-tfe" | ||
| cos_instance_name = var.cos_instance_name != null ? var.cos_instance_name : "${var.instance_name}-cos" | ||
| cos_tags = var.resource_tags | ||
| bucket_name = var.cos_bucket_name != null ? var.cos_bucket_name : "${var.prefix}-tfe-bucket" | ||
| bucket_name = var.cos_bucket_name != null ? var.cos_bucket_name : "${var.instance_name}-bucket" | ||
| add_bucket_name_suffix = true | ||
| create_cos_bucket = true | ||
| retention_enabled = var.cos_retention # disable retention for test environments - enable for stage/prod | ||
|
|
@@ -45,7 +45,7 @@ module "cos" { | |
| module "ocp_vpc" { | ||
| source = "./modules/ocp-vpc" | ||
| region = var.region | ||
| prefix = var.prefix | ||
| instance_name = var.instance_name | ||
| resource_group_id = module.resource_group.resource_group_id | ||
| resource_tags = var.resource_tags | ||
| access_tags = var.access_tags | ||
|
|
@@ -62,7 +62,7 @@ module "icd_postgres" { | |
| source = "terraform-ibm-modules/icd-postgresql/ibm" | ||
| version = "4.4.0" | ||
| resource_group_id = module.resource_group.resource_group_id | ||
| name = var.postgres_instance_name != null ? var.postgres_instance_name : "${var.prefix}-data-store" | ||
| name = var.postgres_instance_name != null ? var.postgres_instance_name : "${var.instance_name}-data-store" | ||
| postgresql_version = "16" # TFE supports up to Postgres 16 (not 17) | ||
| region = var.region | ||
| service_endpoints = "public-and-private" | ||
|
|
@@ -122,7 +122,7 @@ module "tfe_install" { | |
| ######################################################################################################################## | ||
|
|
||
| locals { | ||
| terraform_enterprise_engine_name = var.terraform_enterprise_engine_name != null ? var.terraform_enterprise_engine_name : "${var.prefix}-tfe" | ||
| terraform_enterprise_engine_name = var.terraform_enterprise_engine_name != null ? var.terraform_enterprise_engine_name : "${var.instance_name}-engine" | ||
| } | ||
|
|
||
| resource "ibm_cm_account" "cm_account_instance" { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,8 +9,7 @@ module "vpc" { | |
| region = var.region | ||
| create_vpc = var.existing_vpc_id == null ? true : false | ||
| existing_vpc_id = var.existing_vpc_id | ||
| prefix = var.prefix | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you will need to explicitly set this to |
||
| name = "${var.prefix}-vpc" | ||
| name = "${var.instance_name}-vpc" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think your going to want to ask for var.vpc_name here |
||
| tags = [] | ||
| address_prefixes = { | ||
| zone-1 = ["10.10.10.0/24"] | ||
|
|
@@ -104,7 +103,7 @@ module "openshift" { | |
| count = var.existing_cluster_id == null ? 1 : 0 | ||
| source = "terraform-ibm-modules/base-ocp-vpc/ibm" | ||
| version = "3.73.5" | ||
| cluster_name = var.prefix | ||
| cluster_name = "${var.instance_name}-cluster" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think your going to want to ask for var.cluster_name here |
||
| resource_group_id = var.resource_group_id | ||
| region = var.region | ||
| force_delete_storage = true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,26 +2,21 @@ | |
| # Input Variables | ||
| ######################################################################################################################## | ||
|
|
||
| # variable "ibmcloud_api_key" { | ||
| # type = string | ||
| # description = "The IBM Cloud api key" | ||
| # sensitive = true | ||
| # } | ||
|
|
||
| variable "resource_group_id" { | ||
| type = string | ||
| description = "The Resource Group ID to use for all resources created in this solution (VPC and cluster)" | ||
| default = null | ||
| } | ||
|
|
||
| variable "prefix" { | ||
| variable "instance_name" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. replace this with var.cluster_name and var.vpc_name |
||
| type = string | ||
| description = "Prefix for name of all resource created by this example" | ||
| nullable = false | ||
| description = "The TFE instance name. The resources will be created starting from this, i.e. cluster will be named '[instance_name]_cluster'. Default set to tfe_instance" | ||
| default = "tfe_instance" | ||
| validation { | ||
| error_message = "Prefix must begin and end with a letter and contain only letters, numbers, and - characters." | ||
| condition = can(regex("^([A-z]|[a-z][-a-z0-9]*[a-z0-9])$", var.prefix)) | ||
| error_message = "instance_name must begin and end with a letter and contain only letters, numbers, and - characters." | ||
| condition = can(regex("^([A-z]|[a-z][-a-z0-9]*[a-z0-9])$", var.instance_name)) | ||
| } | ||
| default = "tfe" | ||
| } | ||
|
|
||
| variable "region" { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,6 +80,7 @@ func setupOptions(t *testing.T, prefix string, dir string) *testhelper.TestOptio | |
| ResourceGroup: resourceGroup, | ||
| BestRegionYAMLPath: regionSelectionPath, | ||
| TerraformVars: map[string]interface{}{ | ||
| "instance_name": "dev", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to add here - allow default value to be used. The prefix value is what will make it unique and not have a name clash |
||
| "add_to_catalog": false, | ||
| "postgres_deletion_protection": false, | ||
| }, | ||
|
|
@@ -107,7 +108,7 @@ func setupOptions(t *testing.T, prefix string, dir string) *testhelper.TestOptio | |
| func TestRunCompleteExample(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| options := setupOptions(t, "tfe-complete", completeExampleDir) | ||
| options := setupOptions(t, "compl", completeExampleDir) | ||
|
|
||
| output, err := options.RunTestConsistency() | ||
| assert.Nil(t, err, "This should not have errored") | ||
|
|
@@ -119,7 +120,7 @@ func TestRunUpgradeExample(t *testing.T) { | |
| t.Parallel() | ||
| t.Skip("Skip upgrade test while in Alpha release stage - resume upon official release") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is skipped? Should this be unskipped now? |
||
|
|
||
| options := setupOptions(t, "tfe-complete-upg", completeExampleDir) | ||
| options := setupOptions(t, "compl-upg", completeExampleDir) | ||
|
|
||
| output, err := options.RunTestUpgrade() | ||
| if !options.UpgradeTestSkipped { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,36 +2,29 @@ | |
| # Input Variables | ||
| ######################################################################################################################## | ||
|
|
||
| # variable "ibmcloud_api_key" { | ||
| # type = string | ||
| # description = "The IBM Cloud api key" | ||
| # sensitive = true | ||
| # } | ||
|
|
||
| variable "prefix" { | ||
| variable "instance_name" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should split this out into:
These can all have default values for the module. We need to give users full flexibility in the modules for naming. We don't have to make the DA flexible though. We can just ask for |
||
| type = string | ||
| nullable = true | ||
| description = "The prefix to add to all resources that this solution creates (e.g `prod`, `test`, `dev`). To skip using a prefix, set this value to null or an empty string. [Learn more](https://terraform-ibm-modules.github.io/documentation/#/prefix.md)." | ||
| nullable = false | ||
| description = "The TFE instance name. The resources will be created starting from this, i.e. cluster will be named '[instance_name]_cluster'. Default set to tfe_instance" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't use acronym TFE |
||
| default = "tfe_instance" | ||
|
|
||
| validation { | ||
| # - null and empty string is allowed | ||
| # - Must not contain consecutive hyphens (--): length(regexall("--", var.prefix)) == 0 | ||
| # - Must not contain consecutive hyphens (--): length(regexall("--", var.instance_name)) == 0 | ||
| # - Starts with a lowercase letter: [a-z] | ||
| # - Contains only lowercase letters (a–z), digits (0–9), and hyphens (-) | ||
| # - Must not end with a hyphen (-): [a-z0-9] | ||
| condition = (var.prefix == null || var.prefix == "" ? true : | ||
| alltrue([ | ||
| can(regex("^[a-z][-a-z0-9]*[a-z0-9]$", var.prefix)), | ||
| length(regexall("--", var.prefix)) == 0 | ||
| ]) | ||
| ) | ||
| error_message = "Prefix must begin with a lowercase letter and may contain only lowercase letters, digits, and hyphens '-'. It must not end with a hyphen('-'), and cannot contain consecutive hyphens ('--')." | ||
| condition = alltrue([ | ||
| can(regex("^[a-z][-a-z0-9]*[a-z0-9]$", var.instance_name)), | ||
| length(regexall("--", var.instance_name)) == 0 | ||
| ]) | ||
|
|
||
| error_message = "var.instance_name must begin with a lowercase letter and may contain only lowercase letters, digits, and hyphens '-'. It must not end with a hyphen('-'), and cannot contain consecutive hyphens ('--')." | ||
| } | ||
|
|
||
| validation { | ||
| # must not exceed 16 characters in length | ||
| condition = var.prefix == null || var.prefix == "" ? true : length(var.prefix) <= 16 | ||
| error_message = "Prefix must not exceed 16 characters." | ||
| condition = var.instance_name == null || var.instance_name == "" ? true : length(var.instance_name) <= 16 | ||
| error_message = "instance_name must not exceed 16 characters." | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -109,7 +102,7 @@ variable "add_to_catalog" { | |
|
|
||
| variable "terraform_enterprise_engine_name" { | ||
| type = string | ||
| description = "Name to give to the Terraform Enterprise engine in account catalog settings. Defaults to '{prefix}-tfe' if not set." | ||
| description = "Name to give to the Terraform Enterprise engine in account catalog settings. Defaults to '{instance_name}-engine' if not set." | ||
| default = null | ||
| } | ||
|
|
||
|
|
@@ -150,7 +143,7 @@ variable "existing_cos_instance_id" { | |
| } | ||
|
|
||
| variable "cos_instance_name" { | ||
| description = "Name of COS instance to create. If set to `null`, name will be `{prefix}-tfe`" | ||
| description = "Name of COS instance to create. If set to `null`, name will be `{instance_name}-cos`" | ||
| type = string | ||
| default = null | ||
| validation { | ||
|
|
@@ -160,7 +153,7 @@ variable "cos_instance_name" { | |
| } | ||
|
|
||
| variable "cos_bucket_name" { | ||
| description = "Name of the bucket to create in COS instance. If set to `null`, name will be `{prefix}-tfe-bucket`" | ||
| description = "Name of the bucket to create in COS instance. If set to `null`, name will be `{instance_name}-bucket`" | ||
| type = string | ||
| default = null | ||
| } | ||
|
|
@@ -176,7 +169,7 @@ variable "cos_retention" { | |
| ############################################################################## | ||
|
|
||
| variable "postgres_instance_name" { | ||
| description = "Name of postgres instance to create. If set to `null`, name will be `{prefix}-data-store`" | ||
| description = "Name of postgres instance to create. If set to `null`, name will be `{instance_name}-data-store`" | ||
| type = string | ||
| default = null | ||
| } | ||
|
|
||
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.
no need to duplicate the validation in the example - and give this a default value