Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

[#183035188] Make request timeout and TLS properties configurable#111

Closed
suprajanarasimhan wants to merge 5 commits intowavefrontHQ:masterfrom
suprajanarasimhan:master
Closed

[#183035188] Make request timeout and TLS properties configurable#111
suprajanarasimhan wants to merge 5 commits intowavefrontHQ:masterfrom
suprajanarasimhan:master

Conversation

@suprajanarasimhan
Copy link
Copy Markdown
Contributor

@suprajanarasimhan suprajanarasimhan commented Oct 12, 2022 •
edited
Loading

Related to #6.
Creating this draft in case there is any early feedback to act on.

Before this DRAFT can be finalized on our team's end, the following things are needed:

  • Consider refactoring NewReporter() to take in the entire struct of type configuration to avoid passing multiple parameters to it.
  • Invoke NewSender() and exercise TLS manually (i.e. outside of the test suite) to validate.
  • Updating the README.md to reference examples of invoking newly introduced timeout and TLS-related setters.
  • Update commit message to remove our team's issue tracking ID.
  • [Won't fix] Fix git history (1) To eliminate merge commits (2) To squash all changes into a single commit. -- See how here and do it on branch expose-tls-properties. -- Created a fresh branch instead.

@jbooherl jbooherl marked this pull request as ready for review October 20, 2022 21:15
@jbooherl jbooherl requested a review from keep94 October 20, 2022 21:15
@suprajanarasimhan @warshawd
Make request timeout and TLS properties configurable.
4a17fd6 
Add new configuration options to Readme.

  Signed-off-by: Devon Warshaw <warshawd@vmware.com>
@keep94
Copy link
Copy Markdown
Contributor

keep94 commented Oct 21, 2022

Please resolve merge conflicts.

oppegard
oppegard reviewed Oct 21, 2022
View reviewed changes
@LukeWinikates
Copy link
Copy Markdown
Contributor

LukeWinikates commented Oct 24, 2022

@suprajanarasimhan there's now an example of an end-to-end integration test in senders/integration_test.go - you should be able to extend that pattern to introduce an end-to-end tls configuration test. The current test is just http, but the same httptest package includes a method for starting a TLS server: https://pkg.go.dev/net/http/httptest#NewTLSServer.

@suprajanarasimhan suprajanarasimhan force-pushed the master branch 2 times, most recently from db2091a to d59924f Compare October 27, 2022 00:17
@LukeWinikates
Refactor timeout and add end-to-end TLS test.
5f4afcd 
Signed-off-by: Supraja Narasimhan <suprajanarasimhan@vmware.com>
@suprajanarasimhan
Copy link
Copy Markdown
Contributor Author

suprajanarasimhan commented Nov 3, 2022

Hi @oppegard, thank you for your advice on this PR!

Since I am new to rebasing changes from a main repository onto a fork, I ended up making some undesired merge commits in git history.

Rather than handle the complexity of fixing this in-place on the current branch, I decided to create a fresh branch and #122. The downside is that that PR conversation won't be preserved there. The upside is that I have learned a better workflow next time.

Cc'ing @LukeWinikates in case either of you have feedback for me learn for next time.

@suprajanarasimhan suprajanarasimhan mentioned this pull request Nov 3, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@keep94 keep94 Awaiting requested review from keep94

2 more reviewers

@oppegard oppegard oppegard left review comments

@LukeWinikates LukeWinikates LukeWinikates left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@suprajanarasimhan @keep94 @LukeWinikates @oppegard