Skip to content

fix: Correct capacity reservation target #288

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
Merged
merged 1 commit into from
Aug 13, 2022
Merged

fix: Correct capacity reservation target #288

merged 1 commit into from
Aug 13, 2022

Conversation

bryantbiggs
Copy link
Member

Description

  • Correct capacity reservation target to work without needing to set an open capacity reservation
    • There are two capacity_reservation_specification definitions - one in the instance resource and one in the spot request resource. In the original PR only one of the definition was updated correctly to match the default value type of {}. This is now synchronized across the two resources

Motivation and Context

Breaking Changes

  • No

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

bryantbiggs
bryantbiggs commented Aug 12, 2022
View reviewed changes
examples/complete/main.tf
################################################################################

module "vpc" {
Copy link
Member Author

Choose a reason for hiding this comment

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

Just moved the supporting stuff to the bottom

bryantbiggs
bryantbiggs commented Aug 12, 2022
View reviewed changes
examples/complete/main.tf
@@ -123,10 +51,6 @@ module "ec2_complete" {
cpu_core_count = 2 # default 4
cpu_threads_per_core = 1 # default 2

capacity_reservation_specification = {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is what the fix enables - users do not need to set this if they are not using capacity reservations

bryantbiggs
bryantbiggs commented Aug 12, 2022
View reviewed changes
examples/complete/main.tf
@@ -295,15 +219,13 @@ module "ec2_spot_instance" {
create_spot_instance = true

ami = data.aws_ami.amazon_linux.id
instance_type = "c4.4xlarge"
Copy link
Member Author

Choose a reason for hiding this comment

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

I've got kids to feed 😅

bryantbiggs
bryantbiggs commented Aug 12, 2022
View reviewed changes
examples/complete/main.tf
availability_zone = element(module.vpc.azs, 0)
subnet_id = element(module.vpc.private_subnets, 0)
vpc_security_group_ids = [module.security_group.security_group_id]
placement_group = aws_placement_group.web.id
Copy link
Member Author

Choose a reason for hiding this comment

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

not required on this example, looks to be copy+paste from another example

bryantbiggs
bryantbiggs commented Aug 12, 2022
View reviewed changes
main.tf
@@ -35,14 +35,15 @@ resource "aws_instance" "this" {
ebs_optimized = var.ebs_optimized

dynamic "capacity_reservation_specification" {
Copy link
Member Author

Choose a reason for hiding this comment

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

this was the issue and it now matches

terraform-aws-ec2-instance/main.tf

Lines 184 to 197 in dbf16a2

dynamic "capacity_reservation_specification" {
for_each = length(var.capacity_reservation_specification) > 0 ? [var.capacity_reservation_specification] : []
content {
capacity_reservation_preference = try(capacity_reservation_specification.value.capacity_reservation_preference, null)
dynamic "capacity_reservation_target" {
for_each = try([capacity_reservation_specification.value.capacity_reservation_target], [])
content {
capacity_reservation_id = try(capacity_reservation_target.value.capacity_reservation_id, null)
capacity_reservation_resource_group_arn = try(capacity_reservation_target.value.capacity_reservation_resource_group_arn, null)
}
}
}
}

antonbabenko
antonbabenko approved these changes Aug 13, 2022
View reviewed changes
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Lgtm

@bryantbiggs bryantbiggs merged commit 135145e into terraform-aws-modules:master Aug 13, 2022
antonbabenko pushed a commit that referenced this pull request Aug 13, 2022
@semantic-release-bot
chore(release): version 4.1.4 [skip ci]
668ac71 
### [4.1.4](v4.1.3...v4.1.4) (2022-08-13)

### Bug Fixes

* Correct capacity reservation target ([#288](#288)) ([135145e](135145e))
@antonbabenko
Copy link
Member

This PR is included in version 4.1.4 🎉

@bryantbiggs bryantbiggs deleted the fix/capacity-reservation-target branch November 7, 2022 19:11
@github-actions
Copy link

github-actions bot commented Dec 8, 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 Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@antonbabenko antonbabenko antonbabenko approved these changes

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

Successfully merging this pull request may close these issues.

Issue with plan if not capacity reservation provided
2 participants
@bryantbiggs @antonbabenko