Added support for description to aci_syslog_group module. (DCNE-373) #763
Conversation
DevSinha13
requested review from
akinross,
anvitha-jain,
gmicol,
lhercot,
sajagana,
samiib and
shrsr
as code owners
| @@ -59,6 +61,11 @@ | |||
| - Name of the syslog group | |||
| type: str | |||
| aliases: [ syslog_group, syslog_group_name ] | |||
| description: | |||
There was a problem hiding this comment.
In the format attribute in latest version it is also possible to configure Enhanced Log option rfc5424-ts, same applies to format in aci_syslog_remote_dest. Please find out what this is and add to choices if applicable.
There was a problem hiding this comment.
also the children syslogConsole and syslogFile seem to have format also, is this something that can be configured?
There was a problem hiding this comment.
In UI and model 6.0.9d the severity is restricted to only 3 option for console, please check if we should remove existing values if they do not work? Also verify the models of earlier versions of ACI.
There was a problem hiding this comment.
1.)Ok I have added the new option "rfc5424-ts"
2.)Yes it can be configured and exposed a format attribute for syslogConsole and syslogFile
3.)I have addressed the severity options
| @@ -76,6 +83,7 @@ | |||
| link: https://developer.cisco.com/docs/apic-mim-ref/ | |||
| author: | |||
| - Tim Cragg (@timcragg) | |||
| - Dev Sinha (@DevSinha13) | |||
There was a problem hiding this comment.
update example below with description attribute included
github-actions
bot
changed the title
…n for rfc5424-ts for format
| admin_state=dict(type="str", choices=["enabled", "disabled"]), | ||
| console_logging=dict(type="str", choices=["enabled", "disabled"]), | ||
| console_log_severity=dict(type="str", choices=["alerts", "critical", "debugging", "emergencies", "error", "information", "notifications", "warnings"]), |
There was a problem hiding this comment.
Since some choices here are removed this would be classified as a breaking change. Even though they may not be valid severity levels some users could have these configured in their playbooks.
Instead of removing the values, we could log a warning using module.warn(msg) stating that the choice of severity is no longer valid. Then we revert the choice to the default.
There was a problem hiding this comment.
How would a user have this configured in a working playbook?
There was a problem hiding this comment.
I see, the full set of severity levels only applies to file log and there was always only ever 3 options for console log in the meta. Makes sense to remove them.
There was a problem hiding this comment.
Indeed, these options were never exposed in any version of ACI. I checked the meta files back to 3.0 version. Any usage of these attributes would result in APIC 120 error response that the input is not allowed. We could consider labelling it a bugfix?
There was a problem hiding this comment.
I would still label the PR as a minor change due to the other changes and additional options.
There was a problem hiding this comment.
I thought that the information was taken from the commit messages? Or is it from PR title? The commit messages are marked with [minor_change].
| admin_state=dict(type="str", choices=["enabled", "disabled"]), | ||
| console_logging=dict(type="str", choices=["enabled", "disabled"]), | ||
| console_log_severity=dict(type="str", choices=["alerts", "critical", "debugging", "emergencies", "error", "information", "notifications", "warnings"]), |
There was a problem hiding this comment.
How would a user have this configured in a working playbook?
plugins/modules/aci_syslog_group.py
Outdated
| choices: [ aci, nxos, rfc5424-ts ] | ||
| local_file_log_format: | ||
| description: | ||
| - The format of the local file log messages. If ommitted then uses same format as the format set for syslog messages. |
There was a problem hiding this comment.
what are local_file_log_format and console_log_format set to when O(format) is not provided?
There was a problem hiding this comment.
If there is no value for O(format) then the APIC sets its value to "aci" automatically so by default even if O(format) isn't provided the value for local_file_log_format and console_log_format is "aci" aswell.
| admin_state=dict(type="str", choices=["enabled", "disabled"]), | ||
| console_logging=dict(type="str", choices=["enabled", "disabled"]), | ||
| console_log_severity=dict(type="str", choices=["alerts", "critical", "debugging", "emergencies", "error", "information", "notifications", "warnings"]), |
There was a problem hiding this comment.
I see, the full set of severity levels only applies to file log and there was always only ever 3 options for console log in the meta. Makes sense to remove them.
| admin_state=dict(type="str", choices=["enabled", "disabled"]), | ||
| console_logging=dict(type="str", choices=["enabled", "disabled"]), | ||
| console_log_severity=dict(type="str", choices=["alerts", "critical", "debugging", "emergencies", "error", "information", "notifications", "warnings"]), |
There was a problem hiding this comment.
I would still label the PR as a minor change due to the other changes and additional options.
No description provided.