Skip to content

Conversation

MicahParks
Copy link
Contributor

This pull request expands the supported URL schemes in the parseTarget function to include both HTTP and HTTPS, in addition to the existing gRPC schemes. It updates the scheme validation logic, adjusts default ports and TLS verification accordingly, and improves error handling. The test suite is also updated to cover the new supported schemes and error scenarios.

Expanded protocol support and validation:

  • Added support for http and https schemes in parseTarget, updating the allowedSchemesRe regular expression and related logic to accept these schemes. [1] [2]
  • Introduced a new error variable ErrInvalidTargetScheme for consistent error handling when an unsupported scheme is encountered. [1] [2]

Connection handling improvements:

  • Adjusted default port assignment: grpc and http default to port 80, grpcs and https default to port 443 if the port is not specified.
  • Updated TLS verification logic: enabled for grpcs and https, disabled for grpc and http.

Test coverage enhancements:

  • Added new test cases for valid http and https targets, including correct port and TLS verification expectations.
  • Updated the invalid scheme test case to use an unsupported scheme (ftp) and expect the new ErrInvalidTargetScheme error.

@github-actions github-actions bot added the go label Aug 8, 2025
Copy link

github-actions bot commented Aug 8, 2025

Go test coverage

STATUS ELAPSED PACKAGE COVER PASS FAIL SKIP
🟢 PASS 0.00s github.com/netboxlabs/diode-sdk-go/cmd/diode-replay-dryrun 0.0% 0 0 0
🟢 PASS 1.29s github.com/netboxlabs/diode-sdk-go/diode 20.4% 220 0 0

Total coverage: 75.8%

@github-actions github-actions bot added documentation Improvements or additions to documentation markdown labels Aug 8, 2025
@MicahParks MicahParks changed the title Support HTTP scheme Support HTTP schemes Aug 8, 2025
@MicahParks MicahParks marked this pull request as ready for review August 8, 2025 15:49
Copy link
Contributor

@jajeffries jajeffries left a comment

Choose a reason for hiding this comment

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

LGTM - same comment about breaking changes as here, but I think fine in this case.

Co-authored-by: Michal Fiedorowicz <[email protected]>
@MicahParks MicahParks merged commit 0933756 into develop Aug 11, 2025
6 checks passed
@MicahParks MicahParks deleted the schemes-and-ports branch August 11, 2025 15:48
@MicahParks MicahParks changed the title Support HTTP schemes feat: Support HTTP schemes Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation go markdown

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants