Skip to content
This repository has been archived by the owner on Jul 16, 2024. It is now read-only.

update consul clusters to use draining #9

Merged

Conversation

tfhartmann
Copy link
Contributor

This version of the module puts the requirement on the user of the
module to set instances to DRAINING in some fashion. In our case we are
using our sidecar container, which includes a healthcheck which set the
DRAINING State.

updates the sidecar container used.
This keeps this module inline with
FitnessKeeper/terraform-aws-consul-agents

I suggest we bump the major version of this module, although this PR won't break the module when you run terraform, it will change the assumption on how new tasks get added.

This keeps this module inline with
FitnessKeeper/terraform-aws-consul-agents
This version of the module puts the requirement on the user of the
module to set instances to DRAINING in some fashion. In our case we are
using our sidecar container, which includes a healthcheck which set the
DRAINING State.
@tfhartmann tfhartmann requested a review from a team as a code owner March 15, 2018 20:26
Copy link
Contributor

@hakamadare hakamadare left a comment

Choose a reason for hiding this comment

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

i agree with cutting a new major release; if i understand correctly, this is a breaking change, b/c it changes the names of the parameters that the module accepts

@hakamadare
Copy link
Contributor

bitmoji

@tfhartmann
Copy link
Contributor Author

i agree with cutting a new major release; if i understand correctly, this is a breaking change, b/c it changes the names of the parameters that the module accepts

Yes! Also that! In addition the old version of the module somewhat handled making sure the cluster would survive the instances being cycled under the ASG's by allowing you to span multiple ASG's *and *multiplying the desired task count by two, in this version the desired count is the correct count, so if you ask for a count of 3, you'll get three tasks, and we leave it to another facility to set the instances to draining. In this case, we provide all hooks to do this, but the parameter definitions = ["ecs-cluster"] still needs to be set. Actually in typing that, I'm going to change the default value of definitions to the above, so that some trying out the module with the default settings will get a sense of how that all works.

@tfhartmann tfhartmann merged commit 3300f84 into master Mar 16, 2018
@tfhartmann tfhartmann deleted the tfhartmann/PLAT-2180_update_consul_clusters_to_use_draining branch March 16, 2018 17:38
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.

2 participants