Skip to content

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.

@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 nginx: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.

4 participants