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

feat: Add support for aws_autoscaling_policy #175

Conversation

Sudokamikaze
Copy link
Contributor

@Sudokamikaze Sudokamikaze commented Jan 8, 2022

Description

This PR adds support for aws_autoscaling_policy resource

Support is fully complete and covers all settings mentioned in documentation(https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/autoscaling_policy)

Motivation and Context

I decided to implement support for this resource, as it's a part of ASGs, brings (important in my opinion) feature to scale-in / scale out based on CPU load metric (predefined_metric_type = ASGAverageCPUUtilization)

How Has This Been Tested?

  • [ *] I have tested and validated these changes using one or more of the provided examples/* projects
    I tested this PR creating ASG with addition of following block of code:
  create_scaling_policy = true
  scaling_policies = {
    was-avg-cpu-policy-greater-than-xx = {
      policy_type               = "TargetTrackingScaling"
      estimated_instance_warmup = 1200
      target_tracking_configuration = {
        predefined_metric_specification = {
          predefined_metric_type = "ASGAverageCPUUtilization"
        }
        target_value = 50.0
      }
    }
  }

In terraform plan I see following result:

  # module.workers-ami-test.module.autoscaling.aws_autoscaling_policy.this["was-avg-cpu-policy-greater-than-xx"] will be created"aws_autoscaling_policy" "this" {
      + arn                       = (known after apply)
      + autoscaling_group_name    = "workers-ami-test"
      + estimated_instance_warmup = 1200
      + id                        = (known after apply)
      + metric_aggregation_type   = (known after apply)
      + name                      = "was-avg-cpu-policy-greater-than-xx"
      + policy_type               = "TargetTrackingScaling"

      + target_tracking_configuration {
          + disable_scale_in = false
          + target_value     = 50

          + predefined_metric_specification {
              + predefined_metric_type = "ASGAverageCPUUtilization"
            }
        }
    }

I also made a test without it, as variables have default value of false and null, it does not break anything

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.

Good start. Please update the code in the examples and docs in README.

@Sudokamikaze
Copy link
Contributor Author

Good start. Please update the code in the examples and docs in README.

Agreed, I'll update them and will let you know once it's done, thank you :)

@Qanop
Copy link

Qanop commented Jan 10, 2022

Great message! I was just looking for this solution today while modifying the code.

@Sudokamikaze
Copy link
Contributor Author

@antonbabenko I've added example based on average CPU load and added missing outputs

@Sudokamikaze Sudokamikaze force-pushed the feature/add_target_scaling_policies branch from c19985d to dc37c99 Compare January 10, 2022 14:10
outputs.tf Outdated Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@Sudokamikaze
Copy link
Contributor Author

@bryantbiggs Hey, TYVM for the review & changes

I've implemented them, slightly tested by plan & updated documentation slightly

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 (almost)

README.md Outdated
create_scaling_policy = true
scaling_policies = {
my-policy = {

Copy link
Member

Choose a reason for hiding this comment

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

Could you put at least a smallest possible (required set) example into my-policy ?

variables.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
examples/complete/main.tf Outdated Show resolved Hide resolved
examples/complete/main.tf Show resolved Hide resolved
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.

Looks pretty good. There are few remaining comments, also please update outputs in examples, and make CI green&passing. Thanks!

outputs.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@Sudokamikaze
Copy link
Contributor Author

Looks pretty good. There are few remaining comments, also please update outputs in examples, and make CI green&passing. Thanks!

BTW, I'm seeing First-time contributors need a maintainer to approve running workflows. Learn more. each time, it could be much better if those linters could run without approval, so I could fix it before asking you to re-review

Also I forgot to mention, in previous review round, I implemented target_value as required argument for both blocks (to align more with official documentation)

@antonbabenko
Copy link
Member

This is how GitHub is working for us. You don't need to wait for GitHub Actions to run after you commit, because you can run pre-commit run -a to make all required changes locally before committing, and GitHub Actions will do the same and it will be green afterward. Also, you can run this once - pre-commit install --install-hooks and then run git commit normally.

I've just run it locally.

Thank you very much for this PR! I am going to merge it now.

@antonbabenko antonbabenko merged commit a8ba844 into terraform-aws-modules:master Jan 17, 2022
antonbabenko pushed a commit that referenced this pull request Jan 17, 2022
## [4.11.0](v4.10.0...v4.11.0) (2022-01-17)

### Features

* Add support for aws_autoscaling_policy ([#175](#175)) ([a8ba844](a8ba844))
@antonbabenko
Copy link
Member

This PR is included in version 4.11.0 🎉

@Sudokamikaze
Copy link
Contributor Author

This is how GitHub is working for us. You don't need to wait for GitHub Actions to run after you commit, because you can run pre-commit run -a to make all required changes locally before committing, and GitHub Actions will do the same and it will be green afterward. Also, you can run this once - pre-commit install --install-hooks and then run git commit normally.

I've just run it locally.

Thank you very much for this PR! I am going to merge it now.

Huge thank you! In this PR I learned good practices and looking forward to contribute to your modules, next time it'll be much less to review ;)

@antonbabenko
Copy link
Member

You are more than welcome! We have plenty of work to do in terraform-aws-modules.

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants