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

Small cleanups and fixes #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

mattock
Copy link

@mattock mattock commented Jun 11, 2015

Hi,

The first commit fixes some (trivial) puppet-lint warnings. From what I've read Puppet 4 does not allow references to relative class names, so "include patterdb" won't work, whereas "include ::patterndb" will. For this reason I fixed those.

Puppet-lint also complained about indentation; the resources are currently defined like this:

resource { 'title':
  long_parameter_name => 'value',
  otherparam => 'value2',
}

Whereas puppet-lint wants them to look like this:

resource { 'title':
  long_parameter_name => 'value',
  otherparam          => 'value2',
}

If you want, I can trivially fix these indentation issues also.

The second commit's primary purpose is to fix installation of syslog-ng on Ubuntu 14.04. It is quite likely that installation was broken on other Debian derivatives also. The commit message contains hopefully adequate explanation of what was done and why.

Syslog-ng package failed to install on Ubuntu 14.04 because syslog-ng-core
package conflicted with the rsyslog package. The problem was fixed by ensuring
that rsyslog is removed before syslog-ng installation is attempted. To make
things cleaner and more manageable install and params subclasses were also
added.
@faxm0dem
Copy link
Member

First of all thank you for your interest in this module and for your PR!

  • Knock yourself out for any lint related stuff, I'll merge them as-is as long as they're in a seperate commit (which seems to be the case 👍)
  • About the syslog-ng package: I made this really minimal as we actually use the syslog-ng puppet-module instead, which I strongly recommend
  • I'm surprised that Ubuntu doesn't allow for both rsyslog and syslog-ng to be installed (as is the case for my Debian 8). Are you using the OS default package?

@mattock
Copy link
Author

mattock commented Jun 15, 2015

I'm using the OS default packages. I was also rather surprised that installing syslog-ng did not automatically remove rsyslogd, but that seemed to be the case. So it's not just about not being able to have both installed at the same time.

If a separate syslog-ng module exists, maybe syslog-ng installation should be removed from the patterndb module altogether? I can do that if that's fine with you.

@faxm0dem
Copy link
Member

Out of curiosity, which files conflict between rsyslog and syslog-ng?

I'd prefer to have manage_package set to false by default, and keep the current behaviour: some people use other package sources, which enable keeping both rsyslog and syslog-ng.

Finally, separating the OS defaults in params.pp is nice, so please keep that.

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.

2 participants