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

Fixes splunk/ansible-role-for-splunk#109 #152

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

jewnix
Copy link
Collaborator

@jewnix jewnix commented Nov 22, 2022

Switch to using the lineinfile module instead of a template. This will add the setfacl command to where is belongs instead of in a separate script, which is prone to break.

Copy link
Collaborator

@midnight5ky midnight5ky left a comment

Choose a reason for hiding this comment

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

Just curious, will this insert the line specified each time this task is ran? Wasn't sure if it'll produce duplicates of the line since it just inserts the line before ' endscript'

@jewnix
Copy link
Collaborator Author

jewnix commented Nov 22, 2022

Just curious, will this insert the line specified each time this task is ran? Wasn't sure if it'll produce duplicates of the line since it just inserts the line before ' endscript'

In my testing, it has not put it in twice

@dtwersky dtwersky merged commit 9df2892 into splunk:master Nov 23, 2022
@jewnix jewnix deleted the fix_logrotate branch December 21, 2022 18:02
@zyphermonkey
Copy link
Contributor

@jewnix
Would it be possible to revert this change?

  1. I don't fully understand why this change was made.
    From the issue it seems like some were having issues with their line endings which was most likely cause by their .gitconfig settings and not the template.
  2. It's not best practice (nor am I a fan of) modifying default system config files.
    Especially in the case of logrotate where it specifically gives us a .d directory to make our customization.
  3. Modifying default system config files can lead to inconsistent results.
    Because we have no control over how updates might change this logrotate.d/syslog file it's quite possible a system update will completely reset it destroying the changes made in this merge.
  4. This merge isn't backwards compatible with previous version of the role.
    This doesn't cleanup the previous logrotate.d/splunk_facl file causing duplicate configs for existing systems.

@jewnix
Copy link
Collaborator Author

jewnix commented Jan 25, 2023

@zyphermonkey

  1. I don't fully understand why this change was made.
    From the issue it seems like some were having issues with their line endings which was most likely cause by their .gitconfig settings and not the template.

This was to fix #109 that failed because it did not have the log file directive in the script.

  1. It's not best practice (nor am I a fan of) modifying default system config files.
    Especially in the case of logrotate where it specifically gives us a .d directory to make our customization.

I usually refrain from modifying system files, however I did not find a way to solve this issue without this. Any recommendations would be appreciated.

  1. Modifying default system config files can lead to inconsistent results.
    Because we have no control over how updates might change this logrotate.d/syslog file it's quite possible a system update will completely reset it destroying the changes made in this merge.

I was not able to figure out a working solution. Adding /var/log in the old config file didn;t solve the issue, because no files were rotated, so the postscript did nothing.

  1. This merge isn't backwards compatible with previous version of the role.
    This doesn't cleanup the previous logrotate.d/splunk_facl file causing duplicate configs for existing systems.

Good point. Removing that file should be added

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

4 participants