Skip to content
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

feat: Add "num_suffix_format" variable for instance naming #147

Merged
merged 3 commits into from
Jun 10, 2020

Conversation

ryancraig
Copy link
Contributor

@ryancraig ryancraig commented Mar 12, 2020

Description

  • Added num_suffix_format variable. The default value is -%d so use is optional. If the default value is not overridden the module behaves as it did before I made this change.
  • Added variable reference to tags and volume_tags in main.tf.
  • Added content about this variable to the Inputs section of the README.

Motivation and Context

Some enterprises still maintain policies and naming conventions for resources; they wish to treat resources like pets. For example, I need to adhere to the example naming convention "MyPetEC2Instance01" through "MyPetEC2Instance10" for 10 specified EC2 instances. This change allows such policies to be satisfied if required. This is an optional configuration.

Breaking Changes

No breaking changes. Explicit defaults ensure existing behavior and allow optional use.

How Has This Been Tested?

Manually tested.
example use

module "ec2_instance" {
  # source                 = "terraform-aws-modules/ec2-instance/aws"
  source                 = "git::https://github.com/ryancraig/terraform-aws-ec2-instance.git?ref=develop"
  # version                = "~> 2.0"

  name                                 = var.ec2_instance_name
  use_num_suffix                       = true
  num_suffix_format                    = "%02s"
  instance_count                       = var.ec2_instance_count

  iam_instance_profile                 = var.instance_profile

  ami                                  = var.ami_id
  instance_type                        = var.instance_type
  key_name                             = var.key_name
  monitoring                           = false
  vpc_security_group_ids               = var.security_group_ids
  subnet_id                            = var.subnet_id

  instance_initiated_shutdown_behavior = "stop"
  disable_api_termination              = true

  user_data_base64                     = base64encode(local.user_data)

  tags                                 = merge(var.module_tags, var.common_tags, var.ec2_tags)
}

@bryantbiggs
Copy link
Member

hi @ryancraig - thanks for the PR. could you elaborate on what you are trying to solve with this change; what is the use case?

@ryancraig
Copy link
Contributor Author

ryancraig commented Jun 10, 2020

@bryantbiggs Some enterprises still maintain policies and naming conventions for resources; they wish to treat resources like pets. For example, I need to adhere to the example naming convention "MyPetEC2Instance01" through "MyPetEC2Instance10" for 10 specified EC2 instances. This change allows such policies to be satisfied if required. This is an optional configuration.

example use

module "ec2_instance" {
  # source                 = "terraform-aws-modules/ec2-instance/aws"
  source                 = "git::https://github.com/ryancraig/terraform-aws-ec2-instance.git?ref=develop"
  # version                = "~> 2.0"

  name                                 = var.ec2_instance_name
  use_num_suffix                       = true
  num_suffix_format                    = "%02s"
  instance_count                       = var.ec2_instance_count

  iam_instance_profile                 = var.instance_profile

  ami                                  = var.ami_id
  instance_type                        = var.instance_type
  key_name                             = var.key_name
  monitoring                           = false
  vpc_security_group_ids               = var.security_group_ids
  subnet_id                            = var.subnet_id

  instance_initiated_shutdown_behavior = "stop"
  disable_api_termination              = true

  user_data_base64                     = base64encode(local.user_data)

  tags                                 = merge(var.module_tags, var.common_tags, var.ec2_tags)
}

@bryantbiggs
Copy link
Member

ah ok, yes I have seen this too @ryancraig (unfortunately!). in that case would you mind rebasing/merging master to update your branch and then change the title of your PR to something like feat: add "num_suffix_format" variable for instance naming. this looks ok to me since its backwards compatible

@ryancraig
Copy link
Contributor Author

@bryantbiggs Yes, I will get these changes made. Thank you!

@ryancraig ryancraig changed the title Add num_suffix_format variable feat: add "num_suffix_format" variable for instance naming Jun 10, 2020
@ryancraig
Copy link
Contributor Author

@bryantbiggs I rebased and pushed. Should be good to go. Thanks again!

@ryancraig ryancraig changed the title feat: add "num_suffix_format" variable for instance naming feat: Add "num_suffix_format" variable for instance naming Jun 10, 2020
@ryancraig
Copy link
Contributor Author

Note that this PR does what PR #86 does but with maximal flexibility as compared to PR #86.

@bryantbiggs
Copy link
Member

thanks @ryancraig - I agree; this is the better approach so thank you. @antonbabenko what are your thoughts?

@antonbabenko antonbabenko merged commit 08eb577 into terraform-aws-modules:master Jun 10, 2020
@antonbabenko
Copy link
Member

Great! v2.15.0 has been just released, which also fixes #86 .

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants