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

feat: Remove launch configuration support, add support for ignoring desired_capacity #173

Merged
merged 7 commits into from
Feb 14, 2022
Merged

feat: Remove launch configuration support, add support for ignoring desired_capacity #173

merged 7 commits into from
Feb 14, 2022

Conversation

bryantbiggs
Copy link
Member

@bryantbiggs bryantbiggs commented Dec 23, 2021 •
edited
Loading

Description

  • Remove support for launch configuration
  • Add additional autoscaling group idc (ignore desired changes) as well as a new variable ignore_desired_capacity_changes to allow users to opt into an autoscaling group that will ignore desired_capacity
  • Update variables to expand use of lt_xxx abbreviation to launch_template_xxx
  • See UPGRADE-5.0.md for comprehensive list of changes

Motivation and Context

  • Users have often asked for the addition of ignoring desired_capacity but unfortunately it cannot be parameterized. With the removal of launch configurations, it seems reasonable enough to add in this additional bit of logic in order to accommodate both options
  • There has also been confusion over which settings are launch config vs launch template (user data is the most common one); this change removes any of that ambiguity and confusion
  • In addition to the statement above, with these changes we are now able to replace a lot of the duplicated logic in feat!: Removed support for launch configuration and replace count with for_each terraform-aws-eks#1680 . If this change is merged, I intend to do the following:
    • Update the self_managed_node_group sub-module to remove the launch template and autoscaling group configurations and replace with this module. This is now possible due to the addition of ignore_changes = [desired_capacity]
    • Update the eks_managed_node_group sub-module to remove the launch template configuration and replace with this module. In hindsight, there is no need to duplicate the custom launch template configuration there while this module provides the means to create a standalone launch template without an autoscaling group

With these two changes, we are then able to offload even more complexity and logic from the core EKS module and centralize the changes in the appropriate modules (i.e. - if a new feature is added to launch templates then we can update this module and propagate to the EKS module)

Breaking Changes

  • Yes! Launch configuration support is no longer provided now that AWS has stated that they are no longer adding/updating the APIs for launch configurations (launch templates are the successor). In its place, a second autoscaling group that is identical to the default has been added - the only difference between the two is that the second ASG now ignores desired_capacity changes to allow users to utilize autoscaling without reverting or showing up in Terraform plans/applies.

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects

@bryantbiggs
feat!: remove launch configuration support, add support for ignoring …
6958496 
…`desired_capacity`

BREAKING CHANGE: Launch configuration support is no longer provided now that AWS has stated that they are no longer adding/updating the APIs for launch configurations (launch templates are the successor). In its place, a second autoscaling group that is identical to the default has been added - the only difference between the two is that the second ASG now ignores `desired_capacity` changes to allow users to utilize autoscaling without reverting or showing up in Terraform plans/applies.
@bryantbiggs bryantbiggs changed the title feat!: remove launch configuration support, add support for ignoring desired_capacity feat!: Remove launch configuration support, add support for ignoring desired_capacity Dec 23, 2021
@bryantbiggs bryantbiggs marked this pull request as ready for review December 23, 2021 16:31
@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Feb 11, 2022
@bryantbiggs
Copy link
Member Author

Shhhh

@bryantbiggs bryantbiggs added wip and removed stale labels Feb 11, 2022
@antonbabenko
Copy link
Member

@bryantbiggs
Ops, I see that this one should have been merged around the same time as #179 . Could you finish this one and release version 5.1 right away?

@bryantbiggs bryantbiggs changed the title feat!: Remove launch configuration support, add support for ignoring desired_capacity feat: Remove launch configuration support, add support for ignoring desired_capacity Feb 14, 2022
@bryantbiggs
Copy link
Member Author

bryantbiggs commented Feb 14, 2022 •
edited
Loading

ok should be all set @antonbabenko - I updated the upgrade document to include both this change and the tags change (even though this change will go out as v5.1.0)

antonbabenko
antonbabenko reviewed Feb 14, 2022
View reviewed changes
UPGRADE-5.0.md
### Modified

- `var.tags` was previously a list of maps of `key`, `value`, and `propagate_at_launch`. It has been changed to a standard map of `key` = `value` pairs
- The use of `for_each = lookup(map.value, "attr", null) != null ? [map.value.attr] : []` pattern has been replaced with a more concise, readable pattern of `for_each = can(map.value.attr) ? [map.value.attr] : []`. This is a no-op change for users
Copy link
Member

Choose a reason for hiding this comment

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

Good to know.

Can it be written like this - for_each = try([map.value.attr], []) ?

Copy link
Member

Choose a reason for hiding this comment

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

Will it work? I am going to merge this one like this anyway.

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'll check in a min and open a PR if it does work - should be a no-op anyways

@antonbabenko antonbabenko merged commit 300379d into terraform-aws-modules:master Feb 14, 2022
antonbabenko pushed a commit that referenced this pull request Feb 14, 2022
@semantic-release-bot
chore(release): version 5.1.0 [skip ci]
2e10f98 
## [5.1.0](v5.0.0...v5.1.0) (2022-02-14)

### Features

* Remove launch configuration support, add support for ignoring `desired_capacity` ([#173](#173)) ([300379d](300379d))
@antonbabenko
Copy link
Member

This PR is included in version 5.1.0 🎉

@bryantbiggs bryantbiggs deleted the breaking-change/remove-launch-config branch February 14, 2022 14:53
@bryantbiggs bryantbiggs mentioned this pull request Feb 14, 2022
Merged
1 task
@github-actions
Copy link

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 Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@antonbabenko antonbabenko antonbabenko left review comments

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

Successfully merging this pull request may close these issues.
2 participants
@bryantbiggs @antonbabenko