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

Use DescribeInstance to retrieve filtered AWS instances #29

Merged

Conversation

trjstewart
Copy link
Contributor

Proposed changes

This is an initial implementation to satisfy the minimum requirements of issue #28. It would also close out #6.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • 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

@trjstewart trjstewart force-pushed the feature/get-instances-via-describe-instance branch 2 times, most recently from 2e4ed2d to 055ef27 Compare September 23, 2019 12:47
@trjstewart trjstewart closed this Sep 23, 2019
@trjstewart trjstewart reopened this Sep 23, 2019
@trjstewart trjstewart force-pushed the feature/get-instances-via-describe-instance branch from 90e01a4 to 79c824f Compare September 23, 2019 13:04
@trjstewart
Copy link
Contributor Author

no tests were added as this initial implementation doesn't change the interface for the config file.
also closes PR #6.
refer to issue #29 for further comments.

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@trjstewart thanks for the PR!

Please see a few suggestions.

We will do some additional testing and let you know if we find any problems.

cmd/sync/aws.go Outdated Show resolved Hide resolved
examples/aws.md Outdated Show resolved Hide resolved
examples/aws.md Outdated Show resolved Hide resolved
@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Sep 24, 2019
@trjstewart trjstewart force-pushed the feature/get-instances-via-describe-instance branch from 79c824f to 52959aa Compare September 24, 2019 00:59
@trjstewart
Copy link
Contributor Author

Thanks for the suggestions @pleshakov. I've fixed those few things up.

Copy link
Contributor

@Rulox Rulox left a comment

Choose a reason for hiding this comment

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

lgtm. 🚀

We just need to run some tests and this will be ready.

PS: @pleshakov we need to make a release too, we should wait til this is merged right?

Thanks!

@pleshakov
Copy link
Contributor

@trjstewart thanks! the changes look good. we'll finish the testing shortly and merge it.

@Rulox yep, we'll include this feature into a new release.

For the new release, we're also planning to include support for upstream servers parameter (max_fails, max_conns, fail_timeout, slow_start), which we're currently working on.

@trjstewart
Copy link
Contributor Author

@pleshakov do you have an ETA on getting the new release out? Just trying to decide if I want to build the binary myself and put it on our CDN or wait.

@pleshakov
Copy link
Contributor

@trjstewart the ETA is 2 weeks.

@trjstewart
Copy link
Contributor Author

@pleshakov I'd be happy to take a look at adding the server parm support if you think that would speed things up and make less work for you guys?

@Rulox
Copy link
Contributor

Rulox commented Sep 25, 2019

@trjstewart thanks for your help! We really appreciate it.

I am not sure if the release date can be earlier regarding the time the new features are available (we have the team working on these features already). @pleshakov maybe you have more insights on this?

In the meantime, this PR is good to go, so merging it now! :shipit:

@Rulox Rulox merged commit af1d0c0 into nginxinc:master Sep 25, 2019
@pleshakov
Copy link
Contributor

@trjstewart as Raul mentioned, we're already working on the feature. thanks for your help.

@Rulox Rulox mentioned this pull request Nov 22, 2019
@Rulox
Copy link
Contributor

Rulox commented Nov 26, 2019

@trjstewart If you are still interested in this, this feature (upstream parameters) has been rolled out on the 0.4-1 version, sorry for the delay.

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.

None yet

4 participants