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.

Sign up for GitHub

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

Jump to bottom

Use compact AZ codes in tags #100

Merged
merged 3 commits into from
Sep 11, 2020
Merged

Use compact AZ codes in tags #100

merged 3 commits into from
Sep 11, 2020

Conversation

Nuru
Copy link
Contributor

@Nuru Nuru commented Sep 11, 2020 •
edited
Loading

what

  • When resources are tagged with names that have the Availability Zone appended, use Cloud Posse's new "short" AZ codes
  • Update context.tf and null-label to v0.19.2

why

  • Full Availability Zone (AZ) codes have 2 dashes in them (e.g. "us-east-2") and Cloud Posse's naming convention defaults to using dashes as token separators. Additionally, previous code replaced the dashes in the AZ codes with whatever delimiter was in use for labels. This means that the previous availability zone was treated as 3 tokens when it is more appropriately treated as 1 token. Cloud Posse's new short codes use only digits and lower case letters, ensuring they are always treated as a single token.
  • Stay in sync with other modules

references

Cloud Posse's AWS AZ short codes

@Nuru Nuru requested review from a team as code owners September 11, 2020 05:34
@Nuru Nuru requested review from Makeshift and RothAndrew and removed request for a team September 11, 2020 05:34
@Nuru
Copy link
Contributor Author

Nuru commented Sep 11, 2020

/test all

@Nuru Nuru requested review from aknysh, htplbc and osterman September 11, 2020 05:37
@Nuru
Copy link
Contributor Author

Nuru commented Sep 11, 2020

/test all

@Nuru
Copy link
Contributor Author

Nuru commented Sep 11, 2020

/test all

aknysh
aknysh reviewed Sep 11, 2020
View reviewed changes
main.tf
@@ -22,4 +22,14 @@ data "aws_eip" "nat_ips" {

locals {
use_existing_eips = length(var.existing_nat_ips) > 0
map_map = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this map_map will be included in all modules.
Maybe we should move it to terraform-aws-utils?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map_map is not my first choice, but whatevs. ;)

aknysh
aknysh reviewed Sep 11, 2020
View reviewed changes
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, see comments

@Nuru Nuru merged commit ec96ab4 into master Sep 11, 2020
@Nuru Nuru deleted the abbr branch September 11, 2020 19:26
@cdobbyn cdobbyn mentioned this pull request Oct 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aknysh aknysh aknysh left review comments

@osterman osterman osterman approved these changes

@Makeshift Makeshift Awaiting requested review from Makeshift

@RothAndrew RothAndrew Awaiting requested review from RothAndrew

@htplbc htplbc Awaiting requested review from htplbc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Nuru @osterman @aknysh