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

automatically set ELB_LIST if not provided #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wjordan
Copy link

@wjordan wjordan commented May 10, 2016

This PR removes the need to manually set ELB_LIST when not using an auto-balancer, and removes the step in the README.md where this is required.

In deregister_from_elb.sh, if ELB_LIST is empty this PR calls the existing get_elb_list function to automatically find all load balancers the instance is currently registered to, then writes this list to a temp file (/tmp/elblist).

In register_with_elb.sh, if ELB_LIST is empty this PR reads the list of previously-registered load balancers from the temp file (/tmp/elblist).

The behavior is unchanged when an auto-scaling group is used.

@yubangxi
Copy link
Contributor

Hi,

Thanks for the PR.

Although it might not be the best practice, there could be multiple applications running on the same instance but behind different ELB at the same time. It could be error-prone to automatically deregister and register instance from all ELBs it's bund to. I think it would be helpful if we provide this feature as an opt-in option instead of default behavior.

@wjordan
Copy link
Author

wjordan commented May 12, 2016

I've updated the PR to make this feature opt-in.
Also, I found that this feature requires a fix to the get_elb_list function, which I've added in a separate PR #41.

@suryanarayanan
Copy link
Contributor

Could you please confirm this contribution is under the terms of the Apache 2.0 license. Thanks.

@wjordan
Copy link
Author

wjordan commented May 31, 2016

Confirmed, Apache 2.0 license applies to this contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants