Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/get interface ip nonforwardable network adapters handling #38
base: master
Are you sure you want to change the base?
Feature/get interface ip nonforwardable network adapters handling #38
Changes from 6 commits
01f6a21
fb44e60
b1c7f61
a29c7f2
cb0bec0
ae049f8
aba260c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @deblasis thanks for following up on the team's latest -- we really appreciate your efforts here and receptiveness to feedback.
I may not have been super clear in a previous comment but the line we want to stub out is this one!
We probably want all of the code here to be executed during the tests, except for this line.
This line should look something like
and in the testing we can set up a
FakeNetProvider
. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not sure if we can easily inject a
netProvider
-like dependency here since the exported functions arefunc(string) (string, error)
.It will probably be easiest to declare a global function variable
and stub out global variable
sockaddr.netInterfaces
in testsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there, sorry for the delayed reply, I have been very busy freelancing.
I had a quick look and I believe we are oversimplifying. I'll try to explain what I mean:
We can sure stub out
net.Interfaces
but that means that we are stubbing out the hardware information about our interfaces, what about the software part?We are forgetting the calls to
intf.Addrs()
that look like they are returning network protocol and IP Addresses for the NICs by querying the RIB (Routing Information Base) which returns the address table.https://github.com/hashicorp/go-sockaddr/pull/38/files#diff-9753b911177155deeb161b6f775d09bdbb6cb51fb315e9b6e9bc3130affd0b46R21
that internally call
net.interfaceAddrTable
and
net.addrTable
As far as I understand, in order to be able to stub out all the system calls that are involved in computing the return value of our
sockaddr.GetAllInterfaces()
, I think we should also look into stubbing these other system calls as well...Please correct me if I am wrong and let me know how to proceed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great point @deblasis, I agree that the entire
GetAllInterfaces
function should be mocked.I think exporting new interfaces and implementation structs to mock it are overkill; could we perhaps simply do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kisunji!
I apologize for the delay.
Sounds good, I have updated the PR accordingly.