Repository: zalando-stups/senza

Issue:
Fix Elastigroup healthCheckType
The current implementation always set the Elastigroup's healthCheckType to "ELB" if there are any load balancers, regardless of their types.

The [API clearly states](https://api.spotinst.com/elastigroup/amazon-web-services-2/create/#compute.launchSpecification.healthCheckType) that ELB is for classic ELBs and TARGET_GROUP should be used for ALBs. See [Spotinst's recommendation](https://github.com/zalando-stups/senza/pull/516#pullrequestreview-136726224).

Discussion:
coveralls: 
[![Coverage Status](https://coveralls.io/builds/17987711/badge)](https://coveralls.io/builds/17987711)

Coverage remained the same at 89.549% when pulling **e0c174c48d8db1963e113b7ca5a6f647b20c2f16 on lmineiro:fix-elastigroup-healthchecktype** into **d5477538a198df36914cdd2dbe9e10accb4dec5f on zalando-stups:master**.

jmcs: :+1: 
lmineiro: 👍 

Patch:
diff --git a/requirements.txt b/requirements.txt
index 4b612cc..969c68d 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -7,6 +7,6 @@ dnspython>=1.15.0
 stups-pierone>=1.0.34
 boto3>=1.3.0
 botocore>=1.4.10
-pytest>=2.7.3
+pytest>=3.6.3
 raven
 typing
diff --git a/setup.py b/setup.py
index b038fc5..4d36574 100755
--- a/setup.py
+++ b/setup.py
@@ -131,7 +131,7 @@ def setup_package():
         install_requires=install_reqs,
         setup_requires=['flake8'],
         cmdclass=cmdclass,
-        tests_require=['pytest-cov', 'pytest', 'mock', 'responses'],
+        tests_require=['pytest-cov', 'pytest>=3.6.3', 'mock', 'responses'],
         command_options=command_options,
         entry_points={'console_scripts': CONSOLE_SCRIPTS,
                       'senza.templates': ['bgapp = senza.templates.bgapp',
diff --git a/spotinst/components/elastigroup.py b/spotinst/components/elastigroup.py
index aa916e9..8280ed4 100644
--- a/spotinst/components/elastigroup.py
+++ b/spotinst/components/elastigroup.py
@@ -20,7 +20,12 @@ from spotinst import MissingSpotinstAccount
 
 SPOTINST_LAMBDA_FORMATION_ARN = 'arn:aws:lambda:{}:178579023202:function:spotinst-cloudformation'
 SPOTINST_API_URL = 'https://api.spotinst.io'
-ELASTIGROUP_DEFAULT_STRATEGY = {"risk": 100, "availabilityVsCost": "balanced", "utilizeReservedInstances": True}
+ELASTIGROUP_DEFAULT_STRATEGY = {
+    "risk": 100,
+    "availabilityVsCost": "balanced",
+    "utilizeReservedInstances": True,
+    "fallbackToOd": True,
+}
 ELASTIGROUP_DEFAULT_PRODUCT = "Linux/UNIX"
 
 
@@ -33,7 +38,7 @@ def component_elastigroup(definition, configuration, args, info, force, account_
     """
     definition = ensure_keys(ensure_keys(definition, "Resources"), "Mappings", "Senza", "Info")
     if "SpotinstAccessToken" not in definition["Mappings"]["Senza"]["Info"]:
-        raise click.UsageError("You have to specificy your SpotinstAccessToken attribute inside the SenzaInfo "
+        raise click.UsageError("You have to specify your SpotinstAccessToken attribute inside the SenzaInfo "
                                "to be able to use Elastigroups")
     configuration = ensure_keys(configuration, "Elastigroup")
 
@@ -332,6 +337,7 @@ def extract_load_balancer_name(configuration, elastigroup_config: dict):
 
         if "ElasticLoadBalancer" in configuration:
             load_balancer_refs = configuration.pop("ElasticLoadBalancer")
+            health_check_type = "ELB"
             if isinstance(load_balancer_refs, str):
                 load_balancers.append({
                     "name": {"Ref": load_balancer_refs},
@@ -344,6 +350,7 @@ def extract_load_balancer_name(configuration, elastigroup_config: dict):
                         "type": "CLASSIC"
                     })
         if "ElasticLoadBalancerV2" in configuration:
+            health_check_type = "TARGET_GROUP"
             load_balancer_refs = configuration.pop("ElasticLoadBalancerV2")
             if isinstance(load_balancer_refs, str):
                 load_balancers.append({
@@ -358,16 +365,13 @@ def extract_load_balancer_name(configuration, elastigroup_config: dict):
                     })
 
         if len(load_balancers) > 0:
-            # use ELB health check by default when there are LBs
-            health_check_type = "ELB"
             launch_spec_config["loadBalancersConfig"] = {"loadBalancers": load_balancers}
 
-    if "healthCheckType" in launch_spec_config:
-        health_check_type = launch_spec_config["healthCheckType"]
-    elif "HealthCheckType" in configuration:
-        health_check_type = configuration["HealthCheckType"]
+    health_check_type = launch_spec_config.get("healthCheckType",
+                                               configuration.get("HealthCheckType", health_check_type))
+    grace_period = launch_spec_config.get("healthCheckGracePeriod",
+                                          configuration.get('HealthCheckGracePeriod', 300))
     launch_spec_config["healthCheckType"] = health_check_type
-    grace_period = launch_spec_config.get("healthCheckGracePeriod", configuration.get('HealthCheckGracePeriod', 300))
     launch_spec_config["healthCheckGracePeriod"] = grace_period
 
 
@@ -432,20 +436,16 @@ def extract_instance_types(configuration, elastigroup_config):
     are no SpotAlternatives the Elastigroup will have the same ondemand type as spot alternative
     If there's already a compute.instanceTypes config it will be left untouched
     """
-    elastigroup_config = ensure_keys(ensure_keys(elastigroup_config, "strategy"), "compute")
+    elastigroup_config = ensure_keys(elastigroup_config, "compute")
     compute_config = elastigroup_config["compute"]
-    instance_type = configuration.pop("InstanceType", None)
+
+    if "InstanceType" not in configuration:
+        raise click.UsageError("You need to specify the InstanceType attribute to be able to use Elastigroups")
+    instance_type = configuration.pop("InstanceType")
     spot_alternatives = configuration.pop("SpotAlternatives", None)
     if "instanceTypes" not in compute_config:
-        if not (instance_type or spot_alternatives):
-            raise click.UsageError("You have to specify one of InstanceType or SpotAlternatives")
         instance_types = {}
-        strategy = elastigroup_config["strategy"]
-        if instance_type:
-            instance_types.update({"ondemand": instance_type})
-            strategy.update({"fallbackToOd": True})
-        else:
-            strategy.update({"fallbackToOd": False})
+        instance_types.update({"ondemand": instance_type})
         if spot_alternatives:
             instance_types.update({"spot": spot_alternatives})
         else:


Test patch:
diff --git a/tests/test_elastigroup.py b/tests/test_elastigroup.py
index f77ccc1..022c0a8 100644
--- a/tests/test_elastigroup.py
+++ b/tests/test_elastigroup.py
@@ -442,7 +442,7 @@ def test_load_balancers():
                 "healthCheckGracePeriod": 300,
             }}},
         },
-        {  # 1 application load balancer from Taupage, healthcheck type set to ELB (default grace period)
+        {  # 1 application load balancer from Taupage, healthcheck type set to TARGET_GROUP (default grace period)
             "input": {"ElasticLoadBalancerV2": "bar"},
             "given_config": {},
             "expected_config": {"compute": {"launchSpecification": {
@@ -451,11 +451,12 @@ def test_load_balancers():
                         {"arn": {"Ref": "barTargetGroup"}, "type": "TARGET_GROUP"},
                     ],
                 },
-                "healthCheckType": "ELB",
+                "healthCheckType": "TARGET_GROUP",
                 "healthCheckGracePeriod": 300,
             }}},
         },
-        {  # multiple application load balancers from Taupage, healthcheck type set to ELB (default grace period)
+        {  # multiple application load balancers from Taupage, healthcheck type set to TARGET_GROUP
+            # (default grace period)
             "input": {"ElasticLoadBalancerV2": ["foo", "bar"]},
             "given_config": {},
             "expected_config": {"compute": {"launchSpecification": {
@@ -465,11 +466,11 @@ def test_load_balancers():
                         {"arn": {"Ref": "barTargetGroup"}, "type": "TARGET_GROUP"},
                     ],
                 },
-                "healthCheckType": "ELB",
+                "healthCheckType": "TARGET_GROUP",
                 "healthCheckGracePeriod": 300,
             }}},
         },
-        {  # mixed load balancers from Taupage, healthcheck type set to ELB and custom Taupage grace period
+        {  # mixed load balancers from Taupage, healthcheck type set to TARGET_GROUP and custom Taupage grace period
             "input": {
                 "ElasticLoadBalancer": "foo",
                 "ElasticLoadBalancerV2": "bar",
@@ -483,7 +484,7 @@ def test_load_balancers():
                         {"arn": {"Ref": "barTargetGroup"}, "type": "TARGET_GROUP"},
                     ],
                 },
-                "healthCheckType": "ELB",
+                "healthCheckType": "TARGET_GROUP",
                 "healthCheckGracePeriod": 42,
             }}},
         },
@@ -598,9 +599,11 @@ def test_extract_security_group_ids(monkeypatch):
             assert test_case["expected_sgs"] == got["compute"]["launchSpecification"].get("securityGroupIds")
 
 
-def test_missing_instance_types():
+def test_missing_instance_type():
     with pytest.raises(click.UsageError):
         extract_instance_types({}, {})
+    with pytest.raises(click.UsageError):
+        extract_instance_types({"SpotAlternatives": ["foo", "bar", "baz"]}, {})
 
 
 def test_extract_instance_types():
@@ -608,20 +611,12 @@ def test_extract_instance_types():
         {  # minimum accepted behavior, on demand instance type from typical Senza
             "input": {"InstanceType": "foo"},
             "given_config": {},
-            "expected_config": {"compute": {"instanceTypes": {"ondemand": "foo", "spot": ["foo"]}},
-                                "strategy": {"fallbackToOd": True}},
+            "expected_config": {"compute": {"instanceTypes": {"ondemand": "foo", "spot": ["foo"]}}},
         },
         {  # both on demand instance type from typical Senza and spot alternatives specified
             "input": {"InstanceType": "foo", "SpotAlternatives": ["bar", "baz"]},
             "given_config": {},
-            "expected_config": {"compute": {"instanceTypes": {"ondemand": "foo", "spot": ["bar", "baz"]}},
-                                "strategy": {"fallbackToOd": True}},
-        },
-        {  # only spot alternatives specified
-            "input": {"SpotAlternatives": ["foo", "bar"]},
-            "given_config": {},
-            "expected_config": {"compute": {"instanceTypes": {"spot": ["foo", "bar"]}},
-                                "strategy": {"fallbackToOd": False}},
+            "expected_config": {"compute": {"instanceTypes": {"ondemand": "foo", "spot": ["bar", "baz"]}}},
         },
     ]
     for test_case in test_cases: