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.

Sign up for GitHub

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

Jump to bottom

Check if LoadBalancerName is set #468

Merged
merged 8 commits into from
Jun 14, 2017
Merged

Check if LoadBalancerName is set #468

merged 8 commits into from
Jun 14, 2017

Conversation

a-marcel
Copy link
Contributor

@a-marcel a-marcel commented Jun 12, 2017 •
edited
Loading

Issue Number: #467

The old ELB Configuration has a Property "LoadBalancerName", the new ALB Configuration not. If we support this property in ALB, user can switch from ELB to ALB and will have the same LoadBalancerName.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 89.805% when pulling 87ec2e9 on marcelalburg:master into 786413b on zalando-stups:master.

hjacobs
hjacobs reviewed Jun 12, 2017
View reviewed changes
senza/components/elastic_load_balancer_v2.py Outdated
@@ -73,7 +73,9 @@ def component_elastic_load_balancer_v2(definition,
health_check_path = configuration.get("HealthCheckPath") or '/health'
health_check_port = configuration.get("HealthCheckPort") or configuration["HTTPPort"]

if configuration.get('NameSuffix'):
if configuration.get('LoadBalancerName'):
loadbalancer_name = get_load_balancer_name(configuration["LoadBalancerName"], info["StackVersion"])
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe confusing that LoadBalancerName is now suffixed with StackVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhh, true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you write a new function for this? I don't like to create the same line into a new function.

def get_load_balancer_name(stack_name: str, stack_version: str):

is not working correct and after truncating, there is a possibility that you've a "--"

example:

test-loadbalancer-name-long-red-staging -> test-loadbalancer-name-long-red-[-version]

test-loadbalancer-name-long-red-staging 100 -> test-loadbalancer-name-long--100

(maybe the length of the example is not 100% correct but it show's what i mean).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 89.835% when pulling eeda2a8 on marcelalburg:master into 786413b on zalando-stups:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 89.81% when pulling 402711d on marcelalburg:master into 786413b on zalando-stups:master.

@hjacobs
Copy link
Contributor

hjacobs commented Jun 13, 2017

There are at least flake8 errors:

senza/utils.py:54:1: E101 indentation contains mixed spaces and tabs
senza/utils.py:54:1: W191 indentation contains tabs
senza/utils.py:55:1: E101 indentation contains mixed spaces and tabs
senza/components/elastic_load_balancer.py:5:1: F401 'senza.utils.generate_valid_cloud_name' imported but unused
senza/components/elastic_load_balancer_v2.py:3:1: F401 'senza.components.elastic_load_balancer.generate_valid_cloud_name' imported but unused
senza/components/elastic_load_balancer_v2.py:5:1: E101 indentation contains mixed spaces and tabs
senza/components/elastic_load_balancer_v2.py:5:1: W191 indentation contains tabs
senza/components/elastic_load_balancer_v2.py:5:11: E128 continuation line under-indented for visual indent
senza/components/elastic_load_balancer_v2.py:6:1: E101 indentation contains mixed spaces and tabs
ERROR: InvocationError: '/home/travis/build/zalando-stups/senza/.tox/flake8/bin/python setup.py flake8'

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 89.813% when pulling a7f0020 on marcelalburg:master into 786413b on zalando-stups:master.

@zalando-stups zalando-stups deleted a comment Jun 13, 2017
@zalando-stups zalando-stups deleted a comment Jun 13, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 89.813% when pulling 74b91a8 on marcelalburg:master into 786413b on zalando-stups:master.

tuxlife
tuxlife suggested changes Jun 14, 2017
View reviewed changes
senza/components/elastic_load_balancer_v2.py
@@ -3,6 +3,7 @@
from senza.components.elastic_load_balancer import (ALLOWED_LOADBALANCER_SCHEMES,
get_load_balancer_name,
get_ssl_cert)
from senza.utils import generate_valid_cloud_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you import this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change it, this function should be used in line 77. thanks

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 89.813% when pulling 1a2fafc on marcelalburg:master into 786413b on zalando-stups:master.

@tuxlife
Copy link
Contributor

tuxlife commented Jun 14, 2017

👍

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 89.813% when pulling 4280e7e on marcelalburg:master into 786413b on zalando-stups:master.

@hjacobs
Copy link
Contributor

hjacobs commented Jun 14, 2017

👍

@hjacobs hjacobs merged commit 220c90f into zalando-stups:master Jun 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjacobs hjacobs hjacobs left review comments

@tuxlife tuxlife tuxlife approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@a-marcel @coveralls @hjacobs @tuxlife