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

add instance refresh preferences #115

Merged
merged 9 commits into from
Jul 20, 2023
Merged

add instance refresh preferences #115

merged 9 commits into from
Jul 20, 2023

Conversation

scott-doyland-burrows
Copy link
Contributor

@scott-doyland-burrows scott-doyland-burrows commented Jun 7, 2023

what

Adds skip_matching and auto_rollback to the instance refresh preferences.

Allows the instance refresh preferences to be optional. If the values are not set in the calling module, then the default values are used as per:

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/autoscaling_group#instance_refresh

why

Previously it was not possible to set the skip_matching and auto_rollback.

It was also not possible to omit preferences from the calling module and just use the defaults instead.

With this change we can now set preferences and/or omit preferences that we wish to use the defaults for.

The change should not cause any diffs in current infrastructure.

references

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/autoscaling_group#instance_refresh

examples

It is now possible to use something such as this to use defaults:

  instance_refresh = ({
    strategy = "Rolling"
  })

or this to override certain defaults:

  instance_refresh = ({
    strategy = "Rolling"
    preferences = ({
      min_healthy_percentage = 50
      skip_matching          = true
    })
    triggers = ["tags"]
  })

goruha
goruha previously requested changes Jun 22, 2023
Copy link
Member

@goruha goruha left a comment

Choose a reason for hiding this comment

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

@scott-doyland-burrows How about making preferences also optional?

variables.tf Outdated Show resolved Hide resolved
@Gowiem
Copy link
Member

Gowiem commented Jul 18, 2023

@scott-doyland-burrows this looks good and we'd be happy to get it merged. Can you please do the following and push the changes?

make init
make readme

Thanks!

@Gowiem Gowiem self-requested a review July 18, 2023 17:37
@scott-doyland-burrows
Copy link
Contributor Author

I have run make init and make readme from the root of the repo and pushed the chanes.

@Gowiem
Copy link
Member

Gowiem commented Jul 19, 2023

/terratest

@Gowiem
Copy link
Member

Gowiem commented Jul 19, 2023

Ah @scott-doyland-burrows -- can you update https://github.com/cloudposse/terraform-aws-ec2-autoscale-group/blob/main/versions.tf#L2 to require 1.3+? Otherwise someone could have 1.0 or 1.1 pinned, try to use this module, and then it'll break on optional.

@scott-doyland-burrows
Copy link
Contributor Author

Ah @scott-doyland-burrows -- can you update https://github.com/cloudposse/terraform-aws-ec2-autoscale-group/blob/main/versions.tf#L2 to require 1.3+? Otherwise someone could have 1.0 or 1.1 pinned, try to use this module, and then it'll break on optional.

I have made the change.

@Gowiem
Copy link
Member

Gowiem commented Jul 20, 2023

/terratest

@Gowiem
Copy link
Member

Gowiem commented Jul 20, 2023

Ah apologies @scott-doyland-burrows -- Can you run the make readme again and commit / push the results? The change to the required TF version causes changes in our README.

@scott-doyland-burrows
Copy link
Contributor Author

Ah apologies @scott-doyland-burrows -- Can you run the make readme again and commit / push the results? The change to the required TF version causes changes in our README.

I have run:
make init and make readme

@Gowiem
Copy link
Member

Gowiem commented Jul 20, 2023

/terratest

@Gowiem Gowiem enabled auto-merge (squash) July 20, 2023 11:45
@Gowiem Gowiem dismissed goruha’s stale review July 20, 2023 15:05

Changes have been addressed

@Gowiem Gowiem merged commit 995f0ea into cloudposse:main Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants