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

Add me-central-1, ELB Logging S3 Bucket Policy, Local Zones, Display Names #21

Merged
merged 6 commits into from
Oct 3, 2022
Merged

Add me-central-1, ELB Logging S3 Bucket Policy, Local Zones, Display Names #21

merged 6 commits into from
Oct 3, 2022

Conversation

Nuru
Copy link
Contributor

@Nuru Nuru commented Sep 26, 2022 •
edited
Loading

what

  • Add new me-central-1 zone
  • Add missing Local Zones from us-east-1 and us-west-2
  • Add ELB Logging S3 Bucket Policy output
  • Add region display name map
  • Update minimum Terraform version to 0.14.0

why

  • Maintain complete coverage of regions
  • Improve coverage of local regions
  • To enable ELB access logging to an S3 bucket, you have always needed a region-specific bucket policy. This module provides the account number to use in a policy template based on region, but that is no longer sufficient. There are now 3 different templates for S3 bucket policies, and again which one to use depends on the region. Therefore, to further simplify the task for others, this module can output the full policy JSON, not just the account number.
  • There is no official API to convert from the official region name, like eu-north-1, to a human-friendly name, like "Europe (Stockholm)". This module now outputs a map of such names, derived from the AWS Java SDK.
  • Previous minimum Terraform version 0.13.0 has different formatting, causing formatting checks to fail when compared to code formatted by later versions

references

@Nuru Nuru requested review from a team as code owners September 26, 2022 06:38
@Nuru Nuru requested review from joe-niland and woz5999 September 26, 2022 06:38
@Nuru
Copy link
Contributor Author

Nuru commented Sep 26, 2022

/test all

aknysh
aknysh reviewed Sep 26, 2022
View reviewed changes
README.yaml Outdated
- The `fixed` abbreviations are always exactly 3 characters for regions and 4 characters
for availability zones and local zones, but have some exceptional cases (China, Africa, Asia-Pacific South, US GovCloud)
that have non-obvious abbreviations.
that have non-obvious abbreviations. If a future new region causes a conflict with an established local zone
abbreviation, we may change the local zone abbreviation to to keep the region mappings consistent. For example,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
abbreviation, we may change the local zone abbreviation to to keep the region mappings consistent. For example,
abbreviation, we may change the local zone abbreviation to keep the region mappings consistent. For example,

Benbentwo
Benbentwo previously approved these changes Sep 26, 2022
View reviewed changes
Copy link
Member

@Benbentwo Benbentwo left a comment

Choose a reason for hiding this comment

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

Nice Readme 🙏

aknysh
aknysh reviewed Sep 26, 2022
View reviewed changes
examples/complete/outputs.tf
output "identity_size" {
description = "Size of identity map"
value = local.identity_size
}
Copy link
Member

Choose a reason for hiding this comment

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

add empty lines b/w the outputs

aknysh
aknysh reviewed Sep 26, 2022
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.

a few nitpicks

@Nuru Nuru requested review from aknysh and Benbentwo September 26, 2022 21:33
@Nuru
Copy link
Contributor Author

Nuru commented Sep 26, 2022

/test all

@Nuru Nuru added the minor New features that do not break anything label Sep 26, 2022
@Nuru Nuru enabled auto-merge (squash) September 26, 2022 22:01
Benbentwo
Benbentwo previously approved these changes Sep 30, 2022
View reviewed changes
Copy link
Member

@Benbentwo Benbentwo left a comment

Choose a reason for hiding this comment

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

LGTM

@Nuru
Copy link
Contributor Author

Nuru commented Sep 30, 2022

/test all

@Nuru Nuru disabled auto-merge September 30, 2022 22:55
@Nuru
Copy link
Contributor Author

Nuru commented Sep 30, 2022

/test all

1 similar comment
@Nuru
Copy link
Contributor Author

Nuru commented Sep 30, 2022

/test all

@Nuru
Copy link
Contributor Author

Nuru commented Sep 30, 2022

/test all

@Nuru Nuru requested a review from Benbentwo September 30, 2022 23:33
@Nuru
Copy link
Contributor Author

Nuru commented Sep 30, 2022

/test all

@Nuru Nuru merged commit b30b026 into master Oct 3, 2022
@Nuru Nuru deleted the me-central-1 branch October 3, 2022 18:24
@Nuru Nuru mentioned this pull request May 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@osterman osterman Awaiting requested review from osterman

@mcalhoun mcalhoun Awaiting requested review from mcalhoun

@nitrocode nitrocode Awaiting requested review from nitrocode

@milldr milldr Awaiting requested review from milldr

@dylanbannon dylanbannon Awaiting requested review from dylanbannon

@joe-niland joe-niland Awaiting requested review from joe-niland joe-niland was automatically assigned from cloudposse/contributors

@woz5999 woz5999 Awaiting requested review from woz5999 woz5999 was automatically assigned from cloudposse/contributors

@aknysh aknysh Awaiting requested review from aknysh

@Benbentwo Benbentwo Awaiting requested review from Benbentwo

Assignees
No one assigned
Labels
minor New features that do not break anything
Projects
None yet
Milestone
No milestone
Development

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