-
-
Notifications
You must be signed in to change notification settings - Fork 541
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
feat!: Upgrade module to include capacity providers and bump minimum supported versions #60
feat!: Upgrade module to include capacity providers and bump minimum supported versions #60
Conversation
…roup, and minimum supported versions
…ce` moduel to come
This comment was marked as outdated.
This comment was marked as outdated.
966e5cd
to
4febc6c
Compare
source = "terraform-aws-modules/ecs/aws//modules/ecs-instance-profile" | ||
# Users can pin and stay on v3.5.0 until they able to use the IAM instance | ||
# profile provided through the autoscaling group module | ||
version = "3.5.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try to migrate the instance role and profile from the sub-module in v3.x to the autoscaling group, but the instance profile path and name is different from the role and currently the autoscaling group module makes the assumption that the IAM role and instance profile names and paths are the same. Users can pin their profile to v3.5.0 and continue to use without issue and avoid any disruption when upgrading
examples/ec2/main.tf
Outdated
user_data = base64encode(local.user_data) | ||
ignore_desired_capacity_changes = true | ||
|
||
create_iam_instance_profile = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines L128-L134 are the replacement for the ecs-instance-profile
sub-module, which is provided through the autoscaling group now. The plan is to replace this similar functionality in the EKS module and rely on the ASG module which will reduce quite a bit of code through consolidation (you can see some of that here but that changeset doesn't yet include the removal of the IAM role and instance profile since that was a recent change to the ASG module)
examples/ec2/versions.tf
Outdated
|
||
required_providers { | ||
aws = { | ||
source = "hashicorp/aws" | ||
version = ">= 2.48" | ||
version = ">= 4.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why v4.6 - this is the latest change for ECS service which was released in v4.6. The service sub-module is coming up next which will use this. We could do v4.0 but figured this was more grounded around a feature set that would be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It is totally ok to rely on a newer version in such a major release.
|
||
Configuration in this directory creates: | ||
|
||
- ECS cluster using Fargate (on-demand and spot) capacity provider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the service sub-module is added, its usage will be added here along with example usage of using ALB and NLB to route traffic to tasks.
|
||
⚠️ Module is under active development ⚠️ | ||
|
||
## TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just lays out the rough shape for this sub-module, it will be built out in subsequent PRs
@@ -0,0 +1 @@ | |||
locals {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worlds best module, simple elegance 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, there will be a bit more when more code is there :)
examples/ec2/versions.tf
Outdated
|
||
required_providers { | ||
aws = { | ||
source = "hashicorp/aws" | ||
version = ">= 2.48" | ||
version = ">= 4.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It is totally ok to rely on a newer version in such a major release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR is included in version 4.0.0 🎉 |
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. |
Description
ecs-instance-profile
sub-module has been removed; this functionality is available through theterraform-aws-modules/terraform-aws-autoscaling
module starting with version v6.5.0. Please see theexamples/ec2
example for a demonstration on how to use and integrate with theterraform-aws-autoscaling
module.container_insights
andcapacity_providers
variables have been replaced by new variablesSee UPGRADE-4.0.md for more details
Motivation and Context
Breaking Changes
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull request