Added commit confirm functionality

[kderynski]

Commit confirm functionality added to napalm base. It introduces new optional
argument in commit_config method and new method commit_confirm for
confirmation pending commit. It was briefly discussed under napalm-junos#126

[mirceaulinic]

Hi @kderynski - this looks good to me! As previously said, this feature may take a while the go live as it's an API change.

Two points:

• can we rename the revert_in kwarg to confirmed? I would find it more intuitive
• would you be happy to come with a PR to address that in Junos? Unfortunately I won't have the time to do it myself over the next couple of weeks.

Thank you,
Mircea

[kderynski]

Hi @mirceaulinic,
Sure, I can change it to confirmed and I am happy to make PR for Junos driver. I have already tested it a little bit, so I'll try to create PR over the weekend :)

Best regards,
Kamil

[dbarrosop]

@mirceaulinic , not sure about confirmed=60. What does confirmed mean? Is it going to confirm itself in 60 seconds? In plain english this confirmed option does the opposite of what the wording indicates. If you prefer rollback=60 or auto_rollback=60 or even revert=60 or something else i I am fine with alternatives to revert_in but confirmed feels counterintuitive.

[kderynski]

Hmm, I think that rollback can be misleading as there is already method with the same name. I would vote for auto_rollback as an alternative or maybe revert_in is not a bad name as it seems to be the most descriptive.

BTW I was thinking about time unit for this parameter. Should we use seconds or minutes? Seconds seems to be the best choice but junos accepts rollback time in minutes (minimal time 1 min). IOSXR as far as I remember takes rollback time in seconds. Of course, we can normalize this inside drivers code but what if someone provides 10 sec as rollback time for junos? Should we throw an exception or just time should be increased to possible minimal value? I think we should discuss this aspect as well to have clear guidelines for implementations in all drivers.

[mirceaulinic]

I see your point @dbarrosop - confirmed is indeed less self-explanatory than revert_in.
The reason I would still prefer it is that I suspect network engineers are more used with this term. If you look at:

• junos:

[email protected]# commit confirmed ?
Possible completions:
  <[Enter]>            Execute this command
  <timeout>            Number of minutes until automatic rollback (1..65535)

• iosxr

RP/0/RSP0/CPU0:edge01.flw01(config)#commit confirmed ?
  <30-65535>  Seconds until rollback unless there is a confirming commit
  minutes     Specify the rollback timer in the minutes

Regarding the time unit @kderynski: yes, and I think we should specify the rollback time in minutes. This way we avoid any conflict with Junos and other drivers can easily convert minutes to seconds.

[dbarrosop]

Yeah, I understand that most vendors are just copying junos nomenclature for this and going with the "confirmed" keyword, I think even EOS named it like that on their latest release. That doesn't make it right. I prefer the python philosophy of making things readable as a novel rather than just doing things because someone in the past made a poor naming decision. In addition, it might be the difference between being sued or not by some CLI industry standard maker ;)

Anyway, I don't want to drag this just because we don't agree on the name of the argument. I will delegate the naming decision to @mirceaulinic . Feel free to choose one by any criteria you prefer: going with the industry standard, the readable one or just flipping a coin : )

[kderynski]

@mirceaulinic let me know when you decide which name should we use :) After that, I will change it in this PR and in PR for junos driver.
Using minutes instead of seconds sounds reasonable to me (I thought that IOS XR accepts only seconds). I was thinking originally to convert seconds to minutes (to maintain support for sub-minute rollback for a platform which supports it). Conversion for junos may look like this:

revert_in = 1 if revert_in and revert_in < 60 else revert_in // 60

[mirceaulinic]

@dbarrosop

Yeah, I understand that most vendors are just copying junos nomenclature for this and going with the "confirmed" keyword, I think even EOS named it like that on their latest release. That doesn't make it right. I prefer the python philosophy of making things readable as a novel rather than just doing things because someone in the past made a poor naming decision.

No, it doesn't make it right and I continue to agree with you on this. I just hope it would make it more intuitive for network engineers familiar with the context - as you said because the wrong nomenclature has been already spread out.

In addition, it might be the difference between being sued or not by some CLI industry standard maker ;)

LOL - good one! :)

@kderynski

I thought that IOS XR accepts only seconds

Yes, through the XML agent only seconds seems to allowed. So whoever will implement the changes for the XR driver, will need to convert the rollback time from minutes to seconds, i.e. rollback_time = confirmed * 60.
When updating the doc, please also add a note emphasising that this feature is not supported by all drivers. This is very important to be clear as daylight! Feel free to expand on this as much as you consider it's enough for someone completely new to understand!

Thanks,
Mircea

[mirceaulinic]

It would be great if you'd add in the docstring of commit_config a version of the text you have included in napalm-automation/napalm#355. If could even be the same; I think it's very important to be clear what people should expect from this.

[ktbyers]

I think it is probably better to use confirmed for the name as this name has been adopted as the industry standard for this term (and I think it is reasonably logical i.e. you have to confirm the commit in this amount of time).

Either way we should just choose and get this done.