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

Add support for updating upstream servers #34

Merged
merged 1 commit into from
Nov 8, 2019
Merged

Add support for updating upstream servers #34

merged 1 commit into from
Nov 8, 2019

Conversation

bvighnesha
Copy link

@bvighnesha bvighnesha commented Oct 9, 2019 •
edited

Proposed changes

Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue here in this description (not in the title of the PR).

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

@bvighnesha bvighnesha self-assigned this Oct 9, 2019
@bvighnesha bvighnesha force-pushed the feature/update_upstream_servers branch 3 times, most recently from fd34d40 to aabcebe Compare October 17, 2019 09:57
@bvighnesha bvighnesha changed the title [WIP] Add support for updating upstream servers Add support for updating upstream servers Oct 17, 2019
@Rulox Rulox requested a review from pleshakov October 17, 2019 10:51
Rulox
Rulox suggested changes Oct 17, 2019
View reviewed changes
Copy link

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

I added a suggestion. What happened with the NGINX config files? it seems something happened with the spacing?

client/nginx.go Outdated Show resolved Hide resolved
client/nginx.go Show resolved Hide resolved
@Rulox Rulox requested review from Rulox and removed request for Rulox October 24, 2019 13:42
@Rulox Rulox self-assigned this Oct 24, 2019
pleshakov
pleshakov reviewed Oct 25, 2019
View reviewed changes
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.

@Rulox please see my comments and suggestions

client/nginx.go Outdated Show resolved Hide resolved
client/nginx.go Outdated Show resolved Hide resolved
client/nginx.go Outdated Show resolved Hide resolved
client/nginx_test.go Outdated Show resolved Hide resolved
tests/client_test.go Outdated Show resolved Hide resolved
tests/client_test.go Outdated Show resolved Hide resolved
tests/client_test.go Outdated Show resolved Hide resolved
@Rulox Rulox requested review from pleshakov and Rulox and removed request for Rulox October 25, 2019 14:46
Dean-Coakley
Dean-Coakley reviewed Oct 29, 2019
View reviewed changes
Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

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

Looks good now. Just some small codestyle comments.

tests/client_test.go Outdated Show resolved Hide resolved
client/nginx_test.go Outdated Show resolved Hide resolved
client/nginx_test.go Outdated Show resolved Hide resolved
client/nginx_test.go Outdated Show resolved Hide resolved
pleshakov
pleshakov reviewed Oct 29, 2019
View reviewed changes
client/nginx_test.go Outdated Show resolved Hide resolved
@Rulox Rulox force-pushed the feature/update_upstream_servers branch from 47f08f0 to fb5ad9e Compare November 4, 2019 19:05
Dean-Coakley
Dean-Coakley approved these changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

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

🚀

@Rulox Rulox force-pushed the feature/update_upstream_servers branch from fb5ad9e to 9ac2936 Compare November 5, 2019 12:05
pleshakov
pleshakov requested changes Nov 5, 2019
View reviewed changes
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.

@Rulox looks good. However, there is a bug :(
I left a few suggestion and comments as well

client/nginx.go Show resolved Hide resolved
client/nginx.go Outdated Show resolved Hide resolved
client/nginx.go Outdated Show resolved Hide resolved
client/nginx_test.go Show resolved Hide resolved
client/nginx_test.go Outdated Show resolved Hide resolved
@Rulox Rulox force-pushed the feature/update_upstream_servers branch from 9ac2936 to c1b1ca1 Compare November 6, 2019 13:23
@Rulox Rulox requested a review from pleshakov November 6, 2019 13:23
@Rulox Rulox force-pushed the feature/update_upstream_servers branch from c1b1ca1 to c257253 Compare November 8, 2019 14:34
@Rulox Rulox force-pushed the feature/update_upstream_servers branch from c257253 to afa9917 Compare November 8, 2019 15:42
@Rulox Rulox merged commit e5ee079 into master Nov 8, 2019
@Rulox Rulox deleted the feature/update_upstream_servers branch November 8, 2019 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rulox Rulox Rulox requested changes

@pleshakov pleshakov pleshakov approved these changes

@Dean-Coakley Dean-Coakley Dean-Coakley approved these changes

Assignees

@Rulox Rulox

@bvighnesha bvighnesha

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bvighnesha @pleshakov @Rulox @Dean-Coakley