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

Add support for AIX osfamily and refactoring #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

riton
Copy link

@riton riton commented Nov 5, 2014

In order to enable our AIX hosts to use this module, I had to make some little modifications to your module.
I've tried to modify as little as possible in your code in order to make it modular and easier to maintain. I didn't modify any of your public module interface so you shouldn't have any backward compatibility issue.

What I've done is:

  • Split class base in config, install and defaults.
  • Use top-level variables that enable caller to overwrite default values.
  • Adds support for IBM AIX.
  • Use global variables instead of Linux hard-coded values.

I think that this more modular design could be easier to maintain in the long term with non-Linux O.S support.

My tests were currently only done with puppet apply ... and everything seems to work on Linux (CentOS) and AIX.
I've not updated any of your tests so I expect the C.I build to fail just after this Pull Request will be created. I'd like to have first your opinion about the design I've done before going into further modifications.

I'm waiting for your point of view about this.

Cheers

* Split base in config, install and defaults.
* Use top-level variables that enable caller to overwrite default values.
* Adds support for IBM AIX.
* Use global variables instead of Linux hard-coded values.
@@ -12,4 +12,4 @@ rotate 4
create

# packages drop log rotation information into this directory
include /etc/logrotate.d
include <%= @config_dir %>
Copy link
Author

Choose a reason for hiding this comment

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

Should be scope.lookupvar('::logrotate::config_dir').

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.

1 participant