Skip to content

Conversation

@philrhc
Copy link

@philrhc philrhc commented Nov 29, 2024

attempts to find and try the interface that matches the interface_name
falls back to looping through the interfaces as before

@philrhc
Copy link
Author

philrhc commented Nov 29, 2024

testing this is difficult because:

  • it is difficult to assert from the pyre/node level on the values in zbeacon
  • it is difficult to test network interfaces on different devices

test coverage can improve by extracting the network interface selection to a new file but there are still some cracks...

Copy link
Contributor

@sphaero sphaero left a comment

Choose a reason for hiding this comment

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

I still need to know how the tests are doing. Let me check

pyre/zbeacon.py Outdated
def __init__(self, ctx, pipe, *args, **kwargs):
self.ctx = ctx # ZMQ context
self.pipe = pipe # Actor command pipe
self.ctx = ctx # ZMQ context
Copy link
Contributor

Choose a reason for hiding this comment

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

If you can please undo the whitespace changes? It has nothing to do with the change and it renders the change history useless in case of tracing changes.

Copy link
Author

Choose a reason for hiding this comment

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

thanks, this is especially important for the default values that are set, i've undone these, there are a couple of other random whitespaces which are unchanged
#169

pyre/zbeacon.py Outdated

if self.address:
break
return
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine as a first implementation. Have you looked at how this is handled in zyre(czmq). The interface is selected by either its name or its address. In case the string is a single digit it just uses the index in the interface list.

Copy link
Author

Choose a reason for hiding this comment

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

The interface is selected by either its name or its address. In case the string is a single digit it just uses the index in the interface list.

i have implemented this here: #169

@sphaero
Copy link
Contributor

sphaero commented Dec 4, 2024

I think tests don't run because of deprecation? #165

@sphaero
Copy link
Contributor

sphaero commented Dec 4, 2024

testing this is difficult because:

* it is difficult to assert from the pyre/node level on the values in zbeacon

* it is difficult to test network interfaces on different devices

test coverage can improve by extracting the network interface selection to a new file but there are still some cracks...

I agree. The only decent approach would be to extract the interface name (real or virtual) and try to capture the broadcast. But then it would also need to test on a different interface to see it doesn't capture a broadcast.

I'm fine with merging this. I only don't know if this kills any existing functionality....

@sphaero
Copy link
Contributor

sphaero commented Dec 10, 2024

why was this closed?

@philrhc
Copy link
Author

philrhc commented Dec 10, 2024

i needed to put the master branch in my fork in sync with this, i've pushed this change to another branch and i can't seem to change the source branch after creating a pr

@sphaero
Copy link
Contributor

sphaero commented Dec 10, 2024

Usually you never work from your own master branch. You create a branch for every change you want to do and push that branch to your github repo from which you create the PR.

@philrhc philrhc mentioned this pull request Jan 7, 2025
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.

2 participants