ContainerSolutions / terraform-examples

Simple and idiomatic examples of various Terraform functions and features.
https://containersolutions.github.io/terraform-examples/
159 stars 51 forks source link

Naming convention #61

Closed teszes closed 3 years ago

teszes commented 3 years ago

I would like to reopen the discussion on the naming conventions that we have.

As I understand, the current convention is the following:

I think this has two issues:

Consider the same situation, if the language in question was not Terraform, but Python:

import os

for number in range(15):
    if not os.path.isdir("changeme_test" + f"{number}"):
        os.makedirs("changeme_test" + f"{number}")
        print("created folder : ", "changeme_test" + f"{number}")
    else:
        print("changeme_test" + f"{number}", "folder already exists.")

You could say that there are a few issues with this snippet:

Compare this with a TF snippet from out repo:

# Documentation: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance
resource "aws_instance" "changeme_aws_resource_simple" {
  instance_type = "t2.nano"
  ami           = "ami-0ddbdea833a8d2f0d"
}

This also implies that the thing to change here is the resource name. However, the machine size and the AMI are arguably more important in this context. There is also little documentation to learn what the value of an AMI could be.

To refactor the Python example before, this would be IMO a correct approach:

import os

# Number of directories to be created
DIR_COUNT = 15

# Common directory name prefix
DIR_PREFIX = "test"

for number in range(DIR_COUNT):
    if not os.path.isdir(DIR_PREFIX + f"{number}"):
        os.makedirs(DIR_PREFIX + f"{number}")
        print("created folder : ", DIR_PREFIX + f"{number}")
    else:
        print(DIR_PREFIX + f"{number}", "folder already exists.")

As it explains every variable that can be changed, and also indicates the intention around using them. It also resolves a "magic numbers" code smell.

Conversely, the TF snippet could be changed in the following way:

# Documentation: <link explaining VM sizes>
variable "aws_instance_type" {
  description = "The chosen machine size from the available machine sizes defined by AWS"
}

# Documentation: <link explaining AMIs>
variable "amazon_machine_image_id" {
  description = "The ID of the chosen Amazon Machine Image"
}

# Documentation: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance
resource "aws_instance" "aws_instance_simple" {
  instance_type = var.aws_instance_type
  ami           = var.amazon_machine_image_id
}

This way we could extend documentation to variables as well, and also display all variables that can be changed.

ianmiell commented 3 years ago

Regarding the two points:

I don't think this is worth spending so much time on.

ttarczynski commented 3 years ago

I would add some more explanation to README – for future contributors. Would the following work?


All names that can be change by the user (resources, variables, data items) should be prefixed with "changeme_".

All names should be unique across all examples (resources, variables, data items).