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

Replace Connection.RemoteAddr() by Connection() #171

Merged

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Jun 16, 2023

I am implementing TLS in the example server and found that I need access to the underlying net.Conn. I added Connection() method to access it. Now that we have access to net.Conn, RemoteAddr() is no longer needed, it can be substituted by Connection().RemoteAddr().

Note: this is a breaking change.

@tigrannajaryan tigrannajaryan requested review from a team and andykellr June 16, 2023 21:43
@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Patch coverage: 89.47% and project coverage change: -0.77 ⚠️

Comparison is base (efddaa2) 76.11% compared to head (50dae77) 75.34%.

❗ Current head 50dae77 differs from pull request most recent head c9d16ee. Consider uploading reports for the commit c9d16ee to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   76.11%   75.34%   -0.77%     
==========================================
  Files          24       24              
  Lines        1834     1866      +32     
==========================================
+ Hits         1396     1406      +10     
- Misses        326      342      +16     
- Partials      112      118       +6     
Impacted Files Coverage Δ
client/types/callbacks.go 66.66% <ø> (ø)
server/httpconnection.go 33.33% <0.00%> (ø)
client/internal/mockserver.go 82.35% <85.71%> (+0.31%) ⬆️
server/serverimpl.go 58.92% <94.73%> (+0.37%) ⬆️
client/httpclient.go 95.00% <100.00%> (+0.26%) ⬆️
client/internal/httpsender.go 72.22% <100.00%> (+1.25%) ⬆️
server/callbacks.go 68.18% <100.00%> (-4.55%) ⬇️
server/wsconnection.go 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@andykellr andykellr left a comment

Choose a reason for hiding this comment

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

I think the change is good. I would prefer we use the name "Connection" or "NetworkConnection" (seems redundant), rather than match the package/struct name of the connection. I will let you decide.

@tigrannajaryan
Copy link
Member Author

I think the change is good. I would prefer we use the name "Connection" or "NetworkConnection" (seems redundant), rather than match the package/struct name of the connection. I will let you decide.

Just to be clear, you suggest Connection() as the method name instead of NetConn()?

@andykellr
Copy link
Contributor

I think the change is good. I would prefer we use the name "Connection" or "NetworkConnection" (seems redundant), rather than match the package/struct name of the connection. I will let you decide.

Just to be clear, you suggest Connection() as the method name instead of NetConn()?

Yes

I am implementing TLS in the example server and found that I need access
to the underlying net.Conn. I added NetConn() method to access it.
Now that we have access to net.Conn, RemoteAddr() is no longer needed,
it can be substituted by NetConn().RemoteAddr().
@tigrannajaryan
Copy link
Member Author

I think the change is good. I would prefer we use the name "Connection" or "NetworkConnection" (seems redundant), rather than match the package/struct name of the connection. I will let you decide.

Just to be clear, you suggest Connection() as the method name instead of NetConn()?

Yes

Done.

@tigrannajaryan tigrannajaryan changed the title Replace Connection.RemoteAddr() by NetConn() Replace Connection.RemoteAddr() by Connection() Jun 19, 2023
@tigrannajaryan tigrannajaryan enabled auto-merge (squash) June 19, 2023 15:10
@tigrannajaryan tigrannajaryan merged commit 15441ed into open-telemetry:main Jun 19, 2023
@tigrannajaryan tigrannajaryan deleted the feature/tigran/netconn branch June 19, 2023 15:12
tigrannajaryan added a commit to tigrannajaryan/opamp-go that referenced this pull request Jun 19, 2023
I am implementing TLS in the example server and found that I need access
to the underlying net.Conn. I added Connection() method to access it.
Now that we have access to net.Conn, RemoteAddr() is no longer needed,
it can be substituted by Connection().RemoteAddr().
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