-
Notifications
You must be signed in to change notification settings - Fork 103
Added support for description to aci_syslog_group module. (DCNE-373) #763
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
base: master
Are you sure you want to change the base?
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update example below with description attribute included
…n for rfc5424-ts for format
@@ -30,7 +32,7 @@ | |||
description: | |||
- Severity of events to log to console | |||
type: str | |||
choices: [ alerts, critical, debugging, emergencies, error, information, notifications, warnings ] | |||
choices: [ alerts, critical, emergencies] |
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.
Is there a default severity when unset?
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.
Yes, I described the default value in the documentation.
@@ -44,8 +46,21 @@ | |||
format: | |||
description: | |||
- Format of the syslog messages. If omitted when creating a group, ACI defaults to using aci format. | |||
- C(rfc5424-ts) is only availible starting from version ACI 5.2(8). | |||
type: str | |||
choices: [ aci, nxos, rfc5424-ts ] |
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.
Is there a default format when unset?
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.
Yes, I described the default value in the documentation.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would a user have this configured in a working playbook?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
plugins/modules/aci_syslog_group.py
Outdated
console_log_format: | ||
description: | ||
- Format of the console log messages. | ||
- If unset during creation and O(format) is provided then it is set to the same value as format. If O(format) is not provided it is set to aci. |
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.
- If unset during creation and O(format) is provided then it is set to the same value as format. If O(format) is not provided it is set to aci. | |
- If unset during creation and O(format) is provided then it is set to the same value as format. If O(format) is not provided it is set to C(aci). |
plugins/modules/aci_syslog_group.py
Outdated
local_file_log_format: | ||
description: | ||
- The format of the local file log messages. | ||
- If unset during creation and O(format) is provided then it is set to the same value as format. If O(format) is not provided it is set to aci. |
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.
- If unset during creation and O(format) is provided then it is set to the same value as format. If O(format) is not provided it is set to aci. | |
- If unset during creation and O(format) is provided then it is set to the same value as format. If O(format) is not provided it is set to C(aci). |
plugins/modules/aci_syslog_group.py
Outdated
description: | ||
- Format of the console log messages. | ||
- If unset during creation and O(format) is provided then it is set to the same value as format. If O(format) is not provided it is set to aci. | ||
- C(rfc5424-ts) is only availible starting from version ACI 5.2(8). |
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.
- C(rfc5424-ts) is only availible starting from version ACI 5.2(8). | |
- C(rfc5424-ts) is only available starting from ACI version 5.2(8). |
plugins/modules/aci_syslog_group.py
Outdated
description: | ||
- The format of the local file log messages. | ||
- If unset during creation and O(format) is provided then it is set to the same value as format. If O(format) is not provided it is set to aci. | ||
- C(rfc5424-ts) is only availible starting from version ACI 5.2(8). |
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.
- C(rfc5424-ts) is only availible starting from version ACI 5.2(8). | |
- C(rfc5424-ts) is only available starting from ACI version 5.2(8). |
plugins/modules/aci_syslog_group.py
Outdated
- Format of the syslog messages. If omitted when creating a group, ACI defaults to using aci format. | ||
- C(rfc5424-ts) is only availible starting from version ACI 5.2(8). |
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.
- Format of the syslog messages. If omitted when creating a group, ACI defaults to using aci format. | |
- C(rfc5424-ts) is only availible starting from version ACI 5.2(8). | |
- Format of the syslog messages. | |
- If unset during creation the value defaults to C(aci). | |
- C(rfc5424-ts) is only available starting from ACI version 5.2(8). |
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.
LGTM
No description provided.