Juniper / netconf-java

Java library for NETCONF
Other
76 stars 63 forks source link

Add namespace aware parsing for <hello> #50

Closed GregDThomas closed 3 years ago

GregDThomas commented 3 years ago

This PR is part of the fix for https://github.com/Juniper/netconf-java/issues/49

It a) Introduces a new Hello.java that represents the <hello> NETCONF element. This is generated by using a namespace aware XML parser, so copes with the "nc:" prefix discussed in the ticket b) uses the Hello.java to both generate the client <hello> and parse the server <hello>. This means that the library now will correctly identify the session id when the server is using a nc: prefix. Note that before making a changes, I wrote tests around both sending the client <hello> and receiving the server <hello> - so I'm confident it's not had an deleterious effects. I've also tested it by accessing a local device - and is working OK.

Note; this is only part of the fix for the issue. The <rpc-reply> element needs similar work before the namespace prefix issue can be fully resolved.

So any feedback grateful appreciated!

ydnath commented 3 years ago

@peterjhill please review

peterjhill commented 3 years ago

@GregDThomas , could you please squash all the commits into a single commit. In case you haven't done that before, in your fork, you would do

git rebase -i 5c021c7d54 I think that is the first hash after your feature branch, closest to where you branched.

That will bring up a window with a list of commit hashes and comments. You can change all the picks to "s" for squash.

Then it will allow you to update the commit message. I would update the message with all the changes in the final commit.

Here is an article on squashing: https://medium.com/@slamflipstrom/a-beginners-guide-to-squashing-commits-with-git-rebase-8185cf6e62ec

GregDThomas commented 3 years ago

could you please squash all the commits into a single commit.

OK, I've squashed that down to one commit. I think I've made changes for all your comments except two, which I've responded to above.

Thanks for the feedback!

peterjhill commented 3 years ago

Let me know if you want to make any more changes based on my comments. I'll ping Juniper once we are all set so they can do any final checks and merge.

Thanks for your contributions!

peterjhill commented 3 years ago

Cool, I will ping Juniper.

GregDThomas commented 3 years ago

Nope, no more plans to make changes unless someone strongly objects to the current code!

GregDThomas commented 3 years ago

Hi folks; where are are with this PR? Do you want me to remove the final and/or .editorconfig file? Or is there something else you'd like done? I hope to get to the next stage of this in the coming week, weather permitting.

Vladislav-Kisliy commented 3 years ago

I have no objections probably somebody else has :man_shrugging:

peterjhill commented 3 years ago

Let me ping Juniper

GregDThomas commented 3 years ago

Thank you for merging.