-
Notifications
You must be signed in to change notification settings - Fork 113
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
Added method to allow custom RPC commands #11
Conversation
@nemith @charl is there any reason not to accept this PR? It's a straight forward method that will probably see common use. (the Travis CI build failures below are really old Go, and have nothing to do with @scottdware code) |
|
||
// RawRPC allows any RPC command to be run on the target. | ||
func RawRPC(command string) RawMethod { | ||
return RawMethod(fmt.Sprintf("%s", command)) |
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.
Why are you using fmt.Sprintf() here? command can only ever be a string and you're not doing any variable interpolation.
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 my bad, I should change that to just reflect return RawMethod(command)
@@ -54,7 +55,7 @@ type RPCError struct { | |||
} | |||
|
|||
func (re *RPCError) Error() string { | |||
return fmt.Sprintf("netconf rpc [%s] '%s'", re.Severity, re.Message) | |||
return fmt.Sprintf("netconf rpc [%s]: %s", re.Severity, strings.Trim(re.Message, "[\r\n]")) |
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.
Can you please show me some cases where you need to remove leading and trailing "[\r\n]"?
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.
@charl @scottdware I've just run into the trailing [\n] on some RawMethod output
Using spew, I can see raw output like this
Text: (string) (len=16) "\nAvailable port\n"
I'm not sure where the [\n] is getting introduced. Go1.7.3 xml package
Here is an example code and output
What advantage does this have over just RawMethod("") which is literatly the same thing? I really think there needs to be a redesign of the API and maybe look at type asserting switch statement to do the right thing automatically. |
So, when I first created my API, I also created the RawRPC command as a wrapper. Digging more into it, I didn't really need to do that and could just assign I'll go and update my program to reflect this. Apologies about the confusion. I was still very new to Go at the time of writing it (still am ;P) |
No, my apologies. I kicked up an old thread, and I didn't even catch this already existed. I do think it would be good for us as a community to consolidate on a single go-netconf package. I'm using your |
To be honest this was the first bit of Go code I wrote myself as well and nearly 3 years later it's API design is not that best. Insteading adding more to get around my crappy API design i would like to hear about plans on making it better. I think i would like an Session.Exec that takes a interface{} that we can type assert. If we are given a string or byte slice then send it raw, if we get an object that can be XML encoded then encode it and send that. If we get something that matches the RPCMethod interface then send that. It would make it all a lot easier. |
@jamesboswell @nemith I completely agree with the both of you. It's something I've thought of as well (moving towards your I'm 100% on board with merging/looking to focus on one repo. |
@nemith see http://stackoverflow.com/a/28027945. I am not too sure the swiss army knife convenience of taking an interface{} that then needs to be type asserted is worth the efficiency penalty. Especially if you'll be calling Session.Exec() a lot in a high (RPC) traffic environment. Do you have some specific use cases that are driving your thoughts around changing the API? One place I think we can improve is to get rid of all the strings and only use byte arrays internally instead of strings. |
encoding/xml uses type assertions/type switches. fmt.Printf uses type assertions/type switches. The extra 10 nanoseconds to handle it isn't going to matter when you are doing a round trip to a router and back. And yes that is in NANOseconds. You wouldn't put this into a tight loop or anything else important and you are going to hit the issue when you encode the xml anyway.
Yes it's not very well designed and I would like to provide a way to do encoding/decoding as an interface making it even more transparent to get results.
Why? This smells of premature optimization. string to bytes should be a zero allocation operation as of Go 1.4 or Go 1.5 (or maybe Go 1.3). Do you have an example on where this would matter at all? |
@nemith point taken on the ns saving. Thinking about this from a lib user's perspective you'd want the lib to be as fast as possible becuase of the RTT to the router which you cannot control. Maybe my use case (high volume RPCs) is a little different to most. For my purposes I want go-netconf to as fast (memory efficient as possible and generate as little garbage as possible) as possible. Maybe a discussion on a new API or internals should live elsewhere? I have created #24 for this and seeded it with your thoughts. |
I've changed my script to reflect using |
This will allow any RPC command (
<get-software-information/>
,<get-chassis-inventory/>
, etc.) to be called similar to how the built in functions work.