search

napalm-automation / napalm-junos

Apache License 2.0
22 stars 42 forks source link

add no_resolve as optional_args for traceroute #189

Closed sincerywaing closed 7 years ago

sincerywaing commented 7 years ago
#187 to save time when we use traceroute, add no-resolve as optional_args.
dbarrosop commented 7 years ago

I don't like having an optional_arg for things like this. Either we assume this behavior everywhere and we hardcode it or we implement an argument on the function itself.

mirceaulinic commented 7 years ago

Either we assume this behavior everywhere and we hardcode it or we implement an argument on the function itself.

We should not hardcode this everywhere. An arg would be preferrable, but I'm afraid that would be another thing that might take several weeks to get done. Or, we can postpone it till we migrate to the unified napalm. Or, we can live with the optional arg till we migrate, then we remove and convert it to proper arg (that latter wouldn't be a good idea either).

sincerywaing commented 7 years ago

Yeah for some use cases people may want ip gets resolved to fqdn, but some cases not.

That's why we may want to provide an option instead of hard code it.

This is also to prevent in some environment fw/router may not have dns connectivity so people can save time.

Sent from my iPhone

On 18 Jul 2017, at 4:41 PM, Mircea Ulinic notifications@github.com wrote:

Either we assume this behavior everywhere and we hardcode it or we implement an argument on the function itself.

We should not hardcode this everywhere. An arg would be preferrable, but I'm afraid that would be another thing that might take several weeks to get done. Or, we can postpone it till we migrate to the unified napalm. Or, we can live with the optional arg till we migrate, then we remove it (that latter wouldn't be a good idea either).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

dbarrosop commented 7 years ago

we can postpone it till we migrate to the unified napalm. Or, we can live with the optional arg till we migrate, then we remove and convert it to proper arg (that latter wouldn't be a good idea either).

If it's possible I'd prefer to wait because I know what "temporary fix" means in our world but I am good either way.

mirceaulinic commented 7 years ago

If it's possible I'd prefer to wait because I know what "temporary fix" means in our world but I am good either way.

I agree. @sincerywaing would be fine for you to wait till we migrate to the reunified structure (so you'd be able to gate this kwarg somewhere around November). Otherwise, please feel free to add this kwarg in napalm-base and all drivers implementing the traceroute method.

sincerywaing commented 7 years ago

no problem for me. I'll close this out and let's revisit this couple of month later. Thanks @mirceaulinic