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

aws.GetPrivateIPsForScalingGroup: Only return InService instances #38

Closed

Conversation

HeyBillFinn
Copy link

Proposed changes

Proposed fix for race condition described in #37.

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
    I tested this change locally, but I didn't add any automated tests.
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
    I didn't update any documentation, and I wonder if this should just be the default behavior (rather than providing a config option). If so, should we bump this to 1.5.0 in case it's a breaking change for current users?
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@pleshakov
Copy link
Contributor

@HeyBillFinn thanks for the PR! we'll review and get back to you shortly

@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Apr 6, 2020
@pleshakov
Copy link
Contributor

Hi @HeyBillFinn

Sorry for the delay!

The PR works well.

However, I would like to extend this feature as described below:

Rather than changing the existing behavior, add a new option for nginx-ags-sync to filter out instances of an auto scaling group that are not in the InService state.

To accomplish that, we introduce a new field as part of the upstream:

upstreams:
  - name: backend1
    autoscaling_group: asg-sync-4-WebserverGroup1-BTY7BPLKYTCW
    port: 80
    kind: http
    in_service: true # use instances that are in InService state only. Default is false.

By default, the in_service is false. This would preserve the existing behavior.

I wonder what you think about it?

Thanks for your proposal and this PR. However, I think it makes sense for us to continue with the implementation of this feature. We'll add the new YAML field, add any necessary tests and documentation.

Since your PR solves the problem, could you use it in the meantime? Meanwhile, we'll start working on the feature. Once we finish it, we'll publish a new 0.5-1 release. We'll make sure to mention you in the changelog.

@HeyBillFinn HeyBillFinn deleted the dev-ensure-instance-in-service-2 branch April 24, 2020 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants