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
Implement chatops-enabled configuration consistency checks #10
Conversation
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
aliases/check_consistency.yaml
Outdated
action_ref: "napalm.check_consistency" | ||
description: "Check consistency of the device's configuration" | ||
formats: | ||
- "check consistency on {{hostname}}" |
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.
@Mierdin Since there would be other packs as well on the ST2 installation, does it make sense to give the alias a keyword at the beginning so that chatops commands for this pack are easily identifiable? Some like napalm check consistency on {{hostname}}
. I normally do this for integrations.
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.
Yeah you're right, will do
Otherwise, changes LGTM... |
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
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.
Minor comments to be addressed.
type: "string" | ||
description: "The hostname of the device to connect to. Driver must be specified if hostname is not in configuration. Hostname without FQDN can be given if device is in configuration." | ||
required: true | ||
position: 0 |
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.
Python actions don't need positions. If it's a python script then, yes. The runner type being python-script can be confusing.
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 that. Unfortunately all the actions in the pack are like this. So I'll address this in a follow-up PR that tackles all of them
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.
htmlout: | ||
type: "boolean" | ||
description: "In addition to the normal output also includes html output." | ||
required: false |
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.
You can probably supply default: false
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.
Same as last comment - this needs to get fixed on all actions
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.
actions/check_consistency.py
Outdated
return "".join(config_file.readlines()) | ||
|
||
except IOError: | ||
self.logger.error("Golden config not present in repo") |
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.
Provide the repo name for easy debugging
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 that, done in 651bcc2
self.logger.error(str(e)) | ||
return (False, str(e)) | ||
|
||
return (True, result) |
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.
Can you paste an example of the diff in description please?
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.
Just realized you meant the PR description. Duh. Posted
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
This PR provides an action, and accompanying action-aliase for performing automated WIRI (what it really is) vs WISB (what it should be) comparisons.
Here's an example:
Also some misc corrections/cleanups.