Adding compatibility for Garderos GRS-based devices. #3417
Conversation
| s[:index_last_occurrence].replace(control_seq, "") | ||
| + "\n" | ||
| + s[index_rest_of_string:] | ||
| ) |
There was a problem hiding this comment.
Do you have examples of what the output looks like before any modification? It seems strange to me that you just remove all of the \r\n\r sequences except for the last one?
There was a problem hiding this comment.
First of all, thanks for reviewing my code.
Back in November, I definitely had several routers that needed this fix - even if this looks strange. Unfortunately, I couldn't reproduce it today.
I suggest that I remove this part for now and submit a new PR. I might need to add it again later on if I discover this is getting an issue again.
There was a problem hiding this comment.
Yeah, if you see the output let me know. I somewhat suspect it is a Netmiko artifact (i.e. the newline conversion isn't working a 100% right and leaves some strange artifacts. I have seen something similar on Cisco NX-OS which I never fully worked out.
| self.exit_config_mode() | ||
| # Return all results | ||
| # Will only be executed if no error occured | ||
| return config_results |
There was a problem hiding this comment.
What issue do you run into with the standard send_config_set() method in Netmiko?
Why did you need to override it? I say this as it is not maintainable to have a custom-config method for each driver so we should really do everything in our power to use the parent method?
Is it needed for this?
# Verify if configuration command executed successfully
if command_result != "Set.":
raise ConfigInvalidException(
'Error executing configuration command "{}". Device said: {}'.format(
command_string, command_result
)
)
There was a problem hiding this comment.
Garderos basically confirms every correct configuration command by the response "Set.". Otherwise, it will respond with a descriptive error message.
grs(config)# helloworld
Unknown.
grs(config)# hello=world
Attribute 'hello' not found in node ''.
grs(config)# system.hello=world
Attribute 'hello' not found in node 'system'.
grs(config)# system.name=???
Attribute 'system.name' incorrect: Incorrect format.
grs(config)# system.name=helloworld
Set.
In my understanding, the official implementation of send_config_set() only offers the possibility to blacklist certain error-related string patterns (using error_pattern), correct? However, it appears impossible to reverse the logic by only whitelisting the response "Set." and treating all other responses as errors.
Technically, in regex this would be possible with negative lookahead. Yet, this would only work if regex is matched against the mere respose of the configuration command. Unfortunately, send_config_set() matches error_pattern against the entire config session output (including responses to previously executed configuration commands), so this appears no solution to me. Please correct me if I'm mistaken.
My intention was to implement a "whitelist" version of send_config_set() since a blacklisting approach might not be exhaustive on Garderos GRS.
After all, I agree that code needs to be maintainable. So my suggestion would be that I stick to the error_pattern approach, only overriding the default regex value. I'll rework it and create a new PR.
There was a problem hiding this comment.
Okay, I think we probably should put a whitelist version of error_pattern where it would error_pattern. In other words, make a version of error_pattern that is a positive match. If the positive match is not encountered then, raise an exception.
And put this in the base code (defaulted to False i.e. disabled), but then we can decide for Garderos-GRS whether to potentially enable it by default for that driver.
There was a problem hiding this comment.
Revisiting this, would it work to make error_pattern=r"(Attribute|Unknown)" (or are there more possible outputs).
Alternatively, we could switch the error_pattern search to only search on new data and not on all of the data. So that way we could do the negative lookahead on 'Set.'.
|
Superseded by #3429 |
Added support for Garderos GRS operating system: https://www.garderos.com/
Tests pass successfully. Warnings refer to deprecation of telnetlib in base_connection.