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

Servers Transport on TCP Configuration #9397

Closed
wants to merge 14 commits into from
Closed

Conversation

wxmbugu
Copy link
Contributor

@wxmbugu wxmbugu commented Sep 29, 2022

What does this PR do?

Adding ServersTransport on Tcp Configuration #7803

Motivation

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

  • The tcp configuration could use the httproundtripper instead of implementing another one by making the httproundtripper global and extinsble and also accomadate for tcp and htttp configuration.

We couldn't figure out how use the tcp configurations on newproxy function

@ldez ldez marked this pull request as draft September 30, 2022 08:21
@kevinpollet kevinpollet changed the title Servers Transport on Tcp Configuration Servers Transport on TCP Configuration Sep 30, 2022
@rtribotte
Copy link
Member

Hello @Wambug,

Thanks for your contribution, and your participation in the Hackaethon!

The state of the PR is unfinished, as it introduces a new "ServersTransport" configuration for TCP services, but it is not in use.
Do you plan to iterate over the PR?

@wxmbugu
Copy link
Contributor Author

wxmbugu commented Oct 3, 2022

Yes,I'm planning to iterate over the pr

@wxmbugu
Copy link
Contributor Author

wxmbugu commented Oct 4, 2022

consul test failure with 404

@rtribotte consul test failure with 404
The reproduced error:

FAIL: consul_catalog_test.go:237: ConsulCatalogSuite.TestSimpleConfigurationWithWatch

consul_catalog_test.go:291:
    c.Assert(err, checker.IsNil)
... value *fmt.wrapError = &fmt.wrapError{msg:"try operation failed: got status code 404, wanted 200", err:(*errors.errorString)(0xc000f1c7f0)} ("try operation failed: got status code 404, wanted 200")

@rtribotte
Copy link
Member

Hello @Wambug,

Sorry for the late reply.

FAIL: consul_catalog_test.go:237: ConsulCatalogSuite.TestSimpleConfigurationWithWatch

Some integration tests are flaky, and the ConsulCatalogSuite has some of them if I remember well.
Did the problem resolve by itself on a second run?

Regarding the changes, at glance, it is still missing the modifications for the proxy to use the transport configuration. As well, the current TCP serversTransport configuration is a copy from the HTTP side and some options don’t make sense for TCP.

Are you still iterating on the PR right now?

@wxmbugu
Copy link
Contributor Author

wxmbugu commented Oct 12, 2022

Yes,I just wanted the review to know the changes to be made ,I'll make the changes on the tcp serversTransport. Thanks and also about the missing modification for the proxy is it possible to guide me about achieving that .

@rtribotte
Copy link
Member

Hello @Wambug,

Yes,I just wanted the review to know the changes to be made ,I'll make the changes on the tcp serversTransport. Thanks and also about the missing modification for the proxy is it possible to guide me about achieving that .

Unfortunately, we did not realize you were still working on the solution and some of our team members finished it.
We are sorry about that. Our plan is to close this PR and open a new PR but keep you both as co-authors.
We will keep you posted when the PR will be open.

We thank you for initiating work on this feature. We really loved having you at the hackathon and hope that you continue to work with us. If you are interested in working with us on any other issues, we are glad to work with you to find a good one.

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

Successfully merging this pull request may close these issues.

4 participants