-
Notifications
You must be signed in to change notification settings - Fork 70
(NETDEV-46) Add logfile to syslog_settings #503
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
Conversation
Customer request. Requires the release of netdev-stdlib and the merging of cisco/cisco-network-node-utils#590 |
@willmeek there is an issue with this PR. If attempting to manage the size of a logfile entry that was not previously set, the provider does not issue any updates to the device and is not idempotent. Manage logfile w/out size syslog_settings {'default':
console => 'unset',
monitor => 'unset',
source_interface => 'unset',
time_stamp_units => 'seconds',
logfile_name => 'testlogfile',
logfile_severity_level => '3',
}
Add logfile size syslog_settings {'default':
console => 'unset',
monitor => 'unset',
source_interface => 'unset',
time_stamp_units => 'seconds',
logfile_name => 'testlogfile',
logfile_severity_level => '3',
logfile_size => '4098',
}
|
|
||
#### Parameters | ||
|
||
##### `name` |
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.
Please add a caveats table and indicate that these properties are being added in version 1.10.0
of the module.
Here is an example:
Also please add a link under the Caveats column in the support matrix.
https://github.com/cisco/cisco-network-puppet-module#resource-platform-support-matrix
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.
Thanks Mike, I have added the Caveat and link
README.md
Outdated
|
||
##### `logfile_size` | ||
|
||
Logging file maximum size [4096-2147483647] |
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.
The range will probably be dynamic on different NX-OS platforms so I would omit the range here.
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.
Thanks Mike, removed the range (which simplifies the validation regex hugely! :) )
86132da
to
5d3f459
Compare
@shermdog Good catch. I had assumed that the purge hash contained the logfile name, however if the size was the only change then this was not the case. Grab from the resource existing settings if that is the case. Added a beaker test. |
5d3f459
to
ca9a096
Compare
@willmeek Have you completed the changes and are just waiting for a final review? |
@mikewiebe The changes should be there, awaiting final review, let me know if there is anything that needs changed / fixed, thanks! :) |
(@resource[:logfile_severity_level] && !@resource[:logfile_name])) | ||
fail ArgumentError, | ||
'This provider requires that a logfile_name and logfile_severity_level are both specified in order '\ | ||
'to set logfile size.' if @resource[:logfile_size] && !@resource[:logfile_name] && !@resource[:logfile_severity_level] |
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.
nit /logfile size/logfile_size/
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.
Thanks Mike, updated
One additional nit, otherwise 👍 Will merge once Rick approves. |
ca9a096
to
22d9309
Compare
@willmeek looks like the default resource output is clashing with current netdev_stdlib type definition: [root@puppet-device-devel ~]# puppet device -v -t nxapi --resource syslog_settings --trace
Info: retrieving resource: syslog_settings from nxapi at file:///root/nxapi.yaml
syslog_settings { 'default':
console => 2,
logfile_name => 'unset',
logfile_severity_level => 'unset',
logfile_size => 'unset',
monitor => '5',
source_interface => ['unset'],
time_stamp_units => 'seconds',
}
It also looks like 'unset' is being configured as a long destination when logfile_name is 'unset', but severity level is set syslog_settings {'default':
console => 'unset',
monitor => 'unset',
source_interface => 'unset',
time_stamp_units => 'seconds',
logfile_name => 'unset',
logfile_severity_level => '3',
logfile_size => 'unset',
}
|
This commit adds the logfile_name, logfile_severity_level and logfile_size fields to syslog_settings for Cisco Nexus
22d9309
to
fe39436
Compare
@shermdog Thanks for the thoroughness :) I have re-added 'unset' to logfile_severity_level in netdev stdlib puppetlabs/netdev_stdlib#47 And have amended the validation here to catch if we are trying to unset without unsetting the logfile_name (to avoid possible idempotency issues) 'unset' for the name not being 'unset' was the resource to property_flush field logic being in the wrong place. |
This commit adds the logfile_name, logfile_severity_level and logfile_size fields to syslog_settings for Cisco Nexus