Open kderynski opened 7 years ago
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
@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.
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.
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:
mircea@edge01.bjm01# commit confirmed ?
Possible completions:
<[Enter]> Execute this command
<timeout> Number of minutes until automatic rollback (1..65535)
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.
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 : )
@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
@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
It would be great if you'd add in the docstring of commit_config
a version of the text you have included in https://github.com/napalm-automation/napalm/pull/355. If could even be the same; I think it's very important to be clear what people should expect from this.
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.
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