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

fix: AMI lookup should only happen when launch template is created #2386

Merged
merged 1 commit into from
Jan 5, 2023
Merged

fix: AMI lookup should only happen when launch template is created #2386

merged 1 commit into from
Jan 5, 2023

Conversation

demolitionmode
Copy link
Contributor

Description

Given that data.aws_ami.eks_default is only used in the aws_launch_template.this resource, which has a conditional var.create_launch_template, the data block has been updated to have the same conditional. This means that var.cluster_version is no longer needed to be passed when var.create_launch_template is set to false (ie. when the available EKS AMIs make no difference to the resources actually being created).

Motivation and Context

Fixes #2385

Breaking Changes

No breaking changes

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

Tested with change made against latest on master branch, and the variables shown in related issue.

Prior to change, omission of cluster_version results in:

│ Error: Invalid template interpolation value
│
│   on .terraform/modules/self_managed_node_group_iam_role/modules/self-managed-node-group/main.tf line 9, in data "aws_ami" "eks_default":
│    9:     values = ["amazon-eks-node-${var.cluster_version}-v*"]
│     ├────────────────
│     │ var.cluster_version is null
│
│ The expression result is null. Cannot include a null value in a string
│ template.

After change, omission of cluster_version is valid

@demolitionmode demolitionmode changed the title Make AMI lookup conditional on creation of launch template feat: Make AMI lookup conditional on creation of launch template Jan 5, 2023
@bryantbiggs bryantbiggs changed the title feat: Make AMI lookup conditional on creation of launch template fix: AMI lookup should only happen when launch template is created Jan 5, 2023
Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

makes sense since the launch template is the only consumer of the resource - thank you!

@bryantbiggs bryantbiggs merged commit 3834935 into terraform-aws-modules:master Jan 5, 2023
@demolitionmode demolitionmode deleted the demolitionmode-2385 branch January 5, 2023 21:04
antonbabenko pushed a commit that referenced this pull request Jan 5, 2023
### [19.5.1](v19.5.0...v19.5.1) (2023-01-05)

### Bug Fixes

* AMI lookup should only happen when launch template is created ([#2386](#2386)) ([3834935](3834935))
@antonbabenko
Copy link
Member

This PR is included in version 19.5.1 🎉

@demolitionmode
Copy link
Contributor Author

Thank you for your quick review! 🙇

@github-actions
Copy link

github-actions bot commented Feb 5, 2023

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 Feb 5, 2023
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.

AMI lookup in self-managed-node-group module should only be required if creating a launch template
3 participants