Skip to content

Conversation

@NickCraver
Copy link
Collaborator

This should make PRs like #1977 and #1425 much easier.

This should make PRs like #1977 and #1425 much easier.
@NickCraver
Copy link
Collaborator Author

@rkarthick I really just want a second set of eyes on this, if you have time to review happy to get it in and unblock ya!

Copy link
Contributor

@rkarthick rkarthick left a comment

Choose a reason for hiding this comment

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

LGTM.

Do we need to modify this:

if (serverType == ServerType.Twemproxy)

to
if (!serverType.SupportsAutoConfigure())

@NickCraver
Copy link
Collaborator Author

@rkarthick Indeed! Missed a file in the branch do-over here, thanks!

Copy link
Contributor

@rkarthick rkarthick left a comment

Choose a reason for hiding this comment

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

+2

@NickCraver NickCraver merged commit 74adca8 into main Feb 10, 2022
@NickCraver NickCraver deleted the craver/proxy-3 branch February 10, 2022 01:43
@NickCraver
Copy link
Collaborator Author

@rkarthick thanks again for taking a poke here and helping us improve - a PR against main should be much more friendly not - can open a new PR or just force push the other!

@rkarthick
Copy link
Contributor

this is great, ty @NickCraver I will open a new PR over main and will send it out later today.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants