Modified prefix/resources naming management #50
Conversation
|
/run pipeline |
|
/run pipeline |
1 similar comment
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
ocofaigh
left a comment
ocofaigh
left a comment
There was a problem hiding this comment.
see comments. If needed lets sync up on a call
| default = "" | ||
| } | ||
|
|
||
| variable "instance_name" { |
There was a problem hiding this comment.
don't expose instance_name in the example as an input
| ) | ||
| } | ||
| default = "tfe" | ||
| default = "" |
There was a problem hiding this comment.
no need to duplicate the validation in the example - and give this a default value
| source = "terraform-ibm-modules/base-ocp-vpc/ibm" | ||
| version = "3.73.5" | ||
| cluster_name = var.prefix | ||
| cluster_name = "${var.instance_name}-cluster" |
There was a problem hiding this comment.
I think your going to want to ask for var.cluster_name here
| existing_vpc_id = var.existing_vpc_id | ||
| prefix = var.prefix | ||
| name = "${var.prefix}-vpc" | ||
| name = "${var.instance_name}-vpc" |
There was a problem hiding this comment.
I think your going to want to ask for var.vpc_name here
| } | ||
|
|
||
| variable "prefix" { | ||
| variable "instance_name" { |
There was a problem hiding this comment.
replace this with var.cluster_name and var.vpc_name
| // Upgrade test (using complete example) | ||
| func TestRunUpgradeExample(t *testing.T) { | ||
| t.Parallel() | ||
| t.Skip("Skip upgrade test while in Alpha release stage - resume upon official release") |
There was a problem hiding this comment.
this is skipped? Should this be unskipped now?
| # 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 |
There was a problem hiding this comment.
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")
| 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" |
| # } | ||
|
|
||
| variable "prefix" { | ||
| variable "instance_name" { |
There was a problem hiding this comment.
we should split this out into:
- tfe_instance_name
- cos_instance_name
- cos_bucket_name
- vpc_name
- cluster_name
- postgres_instance_name
- redis_instance_name
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 prefix and not expose these inputs
| region = var.region | ||
| create_vpc = var.existing_vpc_id == null ? true : false | ||
| existing_vpc_id = var.existing_vpc_id | ||
| prefix = var.prefix |
There was a problem hiding this comment.
you will need to explicitly set this to null I think
Description
Modified prefix/resources naming management, removed prefix from root module and creating logic similar to other DAs in solution/demo
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Modified prefix/resources naming management, removed prefix from root module and creating logic similar to other DAs in solution/demo
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