terraform-aws-modules / terraform-aws-ec2-instance

Terraform module to create AWS EC2 instance(s) resources 🇺🇦
https://registry.terraform.io/modules/terraform-aws-modules/ec2-instance/aws
Apache License 2.0
758 stars 1.88k forks source link

fix: Assignment of the Capacity Reservation id to an instance #282

Closed schniber closed 2 years ago

schniber commented 2 years ago

Description

When trying to call the module with the specification of a Capacity Reservation ID, the module ends up with the following error:

╷ │ Error: Invalid function argument │ │ on .terraform/modules/terraform-aws-ec2-instance/main.tf line 45, in resource "aws_instance" "this": │ 45: capacity_reservation_id = lookup(capacity_reservation_target.value, "capacity_reservation_id", null) │ ├──────────────── │ │ capacity_reservation_target.value is "cr-xxxxxxxx" │ │ Invalid value for "inputMap" parameter: lookup() requires a map as the │ first argument. ╵

As per line 45 analysis, it looks like the bug is in the following statement:

capacity_reservation_id = lookup(capacity_reservation_target, "capacity_reservation_id", null)

it should rather be as follows:

capacity_reservation_id = lookup(capacity_reservation_specification.value.capacity_reservation_target, "capacity_reservation_id", null)

In this case, terraform should lookup in the capacity reservation target map (since it is the iterator in the for_each) rather than in its value for the mapping of the attribute capacity reservation id

Motivation and Context

Fixes #277 and #281

Breaking Changes

No breaking changes, just fixing a typo in line 45 in the main.tf

How Has This Been Tested?

schniber commented 2 years ago

@bryantbiggs : Hello Bryant, I noticed that my logic on the previous PR on the same topic was wrong.

In this PR I have revisited this statement in the main.tf:

capacity_reservation_id = lookup(capacity_reservation_specification.value.capacity_reservation_target, "capacity_reservation_id", null)

I also tested thoroughly using the complete example and this is now satisfactory.

Sorry again for the double review work.

Enjoy your weekend !

schniber commented 2 years ago

Hello @bryantbiggs,

kind reminder for this PR which is ready for review.

Thanks again for your feedback.

Bests.

schniber commented 2 years ago

Hello @bryantbiggs

Thanks for your valuable feedback that I have taken into account.

I have added the example for the targeted capacity reservation together with the simplification of the capacity reservation assignment. it's more readable with try function.

I have also deployed to test and well went through.

Looking forward for the merge if all is okay for you.

Thanks a ton !

Bests.

schniber commented 2 years ago

Hello @bryantbiggs

Would it be possible to review the changes I made upon your feedback ?

Thanks again.

Bests.

antonbabenko commented 2 years ago

This PR is included in version 4.1.2 :tada:

yewton commented 2 years ago

Should the default value of capacity_reservation_specification be updated from null to {} ?

bryantbiggs commented 2 years ago

@yewton good spot - yes, looks like we need to update the variable as well. PRs are welcomed!

schniber commented 2 years ago

I will create a new PR to fix it. Thanks for spotting it !

yewton commented 2 years ago

Thank you!

github-actions[bot] commented 2 years ago

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.