-
Notifications
You must be signed in to change notification settings - Fork 551
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
don't remove file prompt quiet unnecessarily #570
Conversation
@benmaddison I am somewhat against tracking state of this as it is prone to problems. Your solution would also need need to check the current status of the flag ( I would rather just have an |
I don't think it is obvious to the end user that you are adjusting the config on their behalf. I'm inclined to agree with @benmaddison |
We have to enable this automatically (otherwise the napalm-ios code will just break and then there will be a lot of issues created and it will be very difficult to manage). So the solutions I see:
I think item 2 is a reasonable solution...but feel free to chime in. There are certain prerequisites to automation using screen-scraping and one of them is to turn off the things that make automation incredibly hard (like prompting for additional things). **Tracking state is very hard as you can't ensure you restore the state properly. Basically abnormal closures of the script will cause the state not to be restored properly. |
what about having this as a requirement as we have for the archive functionality? |
I am concerned about debugging/troubleshooting failures associated with this (so I think we should automatically do it). I think the side-effects are quite a bit more benign than automatically enabling the I think we should give people a way to turn this behavior off via an optional_args and that should be sufficient. Note, if you are not using inline transfers, NAPALM-IOS also automatically enables I guess we could do something like check this setting immediately at Although I think I would probably rather have config side effects (with the option of disabling them) and make the library easier to use and maintain (than not having config side effects). Anyways we shouldn't do what is proposed in the PR which is trying to track the state of this setting. |
I would say we open up a separate issue on |
I think we should probably change the default on that |
@ktbyers your point regarding state tracking is fair. This can be made safer using a method decorator that implements the |
@benmaddison tracking state is too problematic...so I think we can scratch that option off the list. And I think it probably isn't a pattern NAPALM should do (in general). I didn't understand this below statement?
Maybe you are misunderstanding what I was proposing for the optional argument? The optional argument would just cause NAPALM to not modify 'file prompt' at all. NAPALM would neither enable it nor disable it (i.e. you would have to have your device configured already for 'file prompt quiet'). So you would pass this same optional argument in for all your devices. If this argument wasn't used, it would do what it currently does.
I think the above is acceptable. There is only so much you can do.
I think I disagree with this to a certain extent (though I understand the sentiment). There is a trade-off in how reliable easy to use the library is versus changing things. For some devices it is a config change to disable output paging. This prompting for additional information is really a terminal characteristic (unfortunately it is one that is retained across time). Or more exactly it should be a VTY characteristic and Cisco chose to not make it one. Anyways we will need to decide on what to do on this and also on 'ip scp server enable'. |
I tend to agree with this statement and hence my proposal before but I also agree/understand what ktbyers is saying and sometimes you have to balance usability with cleanness.
This is probably a good compromise |
I'm not at all convinced that we should be touching configs to facilitate operations under the hood unless we have absolutely no choice ( |
@benmaddison On IOS you can change Yes, if you want to do a PR...sounds good. I think we agreed to:
|
@bewing @mirceaulinic I will let you guys chime in...I totally understand the sentiment of not wanting any configuration changes to happen behind the scenes, but am also a bit concerned at additional troubleshooting this might involve (not automatically doing this). So let us know if you have an opinion on it. |
@ktbyers I've added the |
napalm/ios/ios.py
Outdated
@@ -377,6 +382,33 @@ def compare_config(self): | |||
|
|||
return diff.strip() | |||
|
|||
def _file_operation(f): |
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.
Let's rename this to something like _file_prompt_quiet
. _file_operation
is too generic.
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.
Ack
docs/support/ios.rst
Outdated
File Operation Prompts | ||
_____ | ||
|
||
By default IOS will prompt for confirmation on file operations. These prompts need to be disabled before the NAPALM-ios driver requires these to be disabled. |
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.
Second sentence doesn't make sense?
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.
Hahaha, no, it really doesn't. I'll fix it...
docs/support/ios.rst
Outdated
_____ | ||
|
||
By default IOS will prompt for confirmation on file operations. These prompts need to be disabled before the NAPALM-ios driver requires these to be disabled. | ||
This can be controlled using the `auto_file_prompt` optional arguement from version XXX: |
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.
Drop the version XXX as we will probably not remember to update this at the release time (you can add this post release if you want i.e. when you know the version that this change happened in).
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.
Ack
napalm/ios/ios.py
Outdated
self._discard_config() | ||
|
||
@_file_operation | ||
def _discard_config(self): |
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 did you need to make a shadow method here (as opposed to just decorating discard_config
)?
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.
This was an interesting py2 vs py3 issue that I hadn't seen before:
In 2.7, functools.wraps()
doesn't fix the signature of wrapper()
to match the original function.
As a result, test.ios.test_getters.TestGetter.test_method_signatures()
fails. Since that test skips private methods the easiest way to work-around was to decorate a private method and call it from the public one.
If you're aware of a more elegant fix then I'm happy to incorporate that?
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.
Okay, makes sense. No I am not aware of a better solution...so we should just use your solution.
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.
Also tested on real IOS device both with auto_prompt_file True and False.
If
file prompt quiet
is already present in the config at the start of an operation, napalm shouldn't remove it.This is fixed by checking the running-config during
_disable_confirm()
, and setting a flag (IOSDriver._do_enable_confirm
) to indicate whether the configuration line ought to be removed during later calls to_enable_confirm()
.A
RuntimeError
is raised if the attribute does not exist, since this indicates that the methods were called in the incorrect order.The configuration line is also removed from
ignore_strings
within_normalize_compare_config()
since this line will only be present in the diff when it is included in either the candidate and/or (existing) running configs. In that case, a change in that command should show up incoompare_config()
.