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

SocketDiagTCPInfo: Add support for BBR information #572

Merged
merged 1 commit into from
Nov 26, 2020
Merged

SocketDiagTCPInfo: Add support for BBR information #572

merged 1 commit into from
Nov 26, 2020

Conversation

taktv6
Copy link

@taktv6 taktv6 commented Oct 1, 2020

No description provided.

@taktv6
Copy link
Author

taktv6 commented Oct 12, 2020

Any chance I could get some feedback?

@vishvananda
Copy link
Owner

This is looking good to me. Any way to add a test for the functionality? Also, when you are ready to go, please squash your changes down into a single commit.

@taktv6
Copy link
Author

taktv6 commented Oct 20, 2020

Thank you for your feedback. I've refactored the SocketDiagTCPInfo function and added some tests.
Unfortunately testify doesn't support Go 1.12 anymore. What would you like me to use as an alternative?

@leoluk
Copy link

leoluk commented Oct 30, 2020

This would be super useful to have - any news on this?

@taktv6
Copy link
Author

taktv6 commented Nov 9, 2020

@vishvananda feedback pls?

socket_linux.go Show resolved Hide resolved
@aboch
Copy link
Collaborator

aboch commented Nov 24, 2020

LGTM

Please squash these changes in one commit ("Squash and merge" is not enabled here).

@taktv6
Copy link
Author

taktv6 commented Nov 24, 2020

Okay. But any idea about the testify issue?
See #596

@aboch
Copy link
Collaborator

aboch commented Nov 24, 2020

But any idea about the testify issue?

Yes, I'd suggest to remove the testify import from these changes. Check the struct equality in the test code.

I would stick to the builtin go testing package, because of the simplicity of the functions that needs to be tested here and for keeping this small library code base lean.

@taktv6
Copy link
Author

taktv6 commented Nov 24, 2020

Okay. I'll sort that out tomorrow. Thanks!

@aboch
Copy link
Collaborator

aboch commented Nov 26, 2020

LGTM

@aboch aboch merged commit 85be443 into vishvananda:master Nov 26, 2020
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.

4 participants