Skip to content
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

feat: Add support for the global config option preserveFQDN with a new logg… #362

Merged
merged 8 commits into from
Oct 31, 2023

Conversation

MrZmurf
Copy link
Contributor

@MrZmurf MrZmurf commented Oct 24, 2023

…ing_preserve_fqdn variable

Enhancement:

Reason:

Result:

Issue Tracker Tickets (Jira or BZ if any):

@spetrosi spetrosi changed the title Add support for the global config option preserveFQDN with a new logg… feat: Add support for the global config option preserveFQDN with a new logg… Oct 24, 2023
@spetrosi
Copy link
Contributor

Thank you for your contributions @MrZmurf. Please fill in the PR description template in this PR to provide more info about the change so that this change is described in CHANGELOG properly.

Please also update README.md to list logging_preserve_fqdn and add a test for setting logging_preserve_fqdn similar to how logging_mark: true is tested in https://github.com/linux-system-roles/logging/blob/main/tests/tests_basics_files.yml#L32

@MrZmurf
Copy link
Contributor Author

MrZmurf commented Oct 27, 2023

Enhancement: Add support to set the preserveFQDN option in rsyslog configuration.

Reason: There was previously no way to configure the preserveFQDN config option with this role.

Result: Add the optional logging_preserve_fqdn variable to set the preserveFQDN configuration option in syslog. This will make it configurable to use the full FQDN instead of short name in syslog entries.

Added description of the logging_preserve_fqdn variable
logging_preserve_fqdn true will set preserveFQDN="on"
Change description for logging_preserve_fqdn to use true instead of "on" to set preserveFQDN.
@@ -30,4 +30,7 @@ global(
{% if rsyslog_max_message_size is defined %}
maxMessageSize="{{ logging_max_message_size }}"
{% endif %}
{% if logging_preserve_fqdn | d() %}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since logging_preserve_fqdn is in the public API of the role (which are the parameters that users can set), it should be defined in defaults/main.yml as logging_preserve_fqdn: false - then it will always be defined and you can omit the | d()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have set it in defaults.

Copy link
Contributor Author

@MrZmurf MrZmurf Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then again, should it be in global defaults or should I change it to rsyslog_preserve_fqdn and put it in the defaults of the rsyslog role since it is rsyslog specific?

There are also other options in specified at the logging level that probably should be rsyslog specific.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct that this is an rsyslog specific config. However, we broke that API convention about 4 years ago when we decided not to add fluentd support to the logging role, and ever since then, we've been adding rsyslog specific parameters to the top level defaults/main.yml - https://github.com/linux-system-roles/logging/blob/main/defaults/main.yml#L41 - starting here, most of these are rsyslog specific.

So I would prefer putting this in the global defaults as a logging_ parameter, and if/when we ever refactor this role to support some logging provider other than rsyslog, we'll have to figure out how to do that.

@richm
Copy link
Collaborator

richm commented Oct 31, 2023

[citest]

@richm richm merged commit 23dc81e into linux-system-roles:main Oct 31, 2023
23 checks passed
@MrZmurf MrZmurf deleted the patch-2 branch October 31, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants