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

Add source_interface argument to ping #1455

Merged
merged 2 commits into from
May 27, 2021

Conversation

chadell
Copy link
Contributor

@chadell chadell commented May 26, 2021

Some vendors allow the definition of a source interface instead of the source address for the ping command with different keyword. For instance, Junos.
Others don't use different keyword, so both arguments (source and source_interface) will end up using the same one.

@chadell chadell changed the title Add source_interface argument to ping [WIP] Add source_interface argument to ping May 26, 2021
@chadell chadell force-pushed the ping-source-interface branch from 491ff20 to 275c227 Compare May 26, 2021 11:42
@chadell chadell changed the title [WIP] Add source_interface argument to ping Add source_interface argument to ping May 26, 2021
@chadell chadell marked this pull request as ready for review May 26, 2021 11:44
Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

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

Looks good. Any chance you could add the argument for the remaining platforms? (Even without actually implementing, just logging a warning when the argument is being used it would be just fine - see for example like this: https://github.com/napalm-automation/napalm/blob/develop/napalm/junos/junos.py#L1548-L1550)

@chadell
Copy link
Contributor Author

chadell commented May 27, 2021

Looks good. Any chance you could add the argument for the remaining platforms? (Even without actually implementing, just logging a warning when the argument is being used it would be just fine - see for example like this: https://github.com/napalm-automation/napalm/blob/develop/napalm/junos/junos.py#L1548-L1550)

When you say "other platforms" which ones are you thinking on?
Maybe I have skipped some but I tried to add support to all the ping methods in the repository, where junos and eos have a differentiated argument for the source_interface case and for ios and nxos it overloads the already available source argument as it can take or a source_ipaddress or source_interface.

Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

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

You're right @chadell, I assumed the XR drivers implement ping too... Thanks!

@mirceaulinic mirceaulinic merged commit 56b341d into napalm-automation:develop May 27, 2021
@mirceaulinic mirceaulinic added this to the 3.3.1 milestone May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants