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

Parameterize Logstash home directory #324

Merged
merged 1 commit into from
Jan 15, 2017
Merged

Parameterize Logstash home directory #324

merged 1 commit into from
Jan 15, 2017

Conversation

joshuaspence
Copy link
Contributor

Define $logstash::home_dir = '/usr/share/logstash' to avoid needless repetition.

Define `$logstash::home_dir = '/usr/share/logstash'` to avoid needless repetition.
@ninaspitfire
Copy link
Contributor

This is really just declaring a constant rather than a parameter, isn't it?

The same amount of repetition is present, but it's slightly harder to read, because now the reader has to do a "variable lookup" in their head.

@ahharu
Copy link

ahharu commented Jan 13, 2017

I would rather have it added as a parameter on the "logstash" class at init.pp so we can install it wherever we want

@joshuaspence
Copy link
Contributor Author

@jarpy, correct. But defining it as a constant means that it can then be used externally. For example, we have Puppet install development dependencies with /use/share/Logstash/bin/logstash-plugin install --development (see #292) and would prefer not to have to hardcode the path here.

@joshuaspence
Copy link
Contributor Author

@ahharu, this setting doesn't actually change where Logstash is installed because it is installed from a package, so it would be misleading to allow this path to be configurable.

@joshuaspence
Copy link
Contributor Author

This change would also make it easier for the Puppet module to support the installation of Puppet on Windows, if we care about that.

@ninaspitfire
Copy link
Contributor

defining it as a constant means that it can then be used externally

Good point, thanks for helping me get it.

I think I instinctively ignored that possibility, because it feels "wrong" to reference variables across domains like that. That's a feeling about programming in general though. Obviously, in Puppet, this is a pretty normal and common thing to do.

@ninaspitfire ninaspitfire merged commit 937a27c into voxpupuli:master Jan 15, 2017
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.

3 participants