Issue 2864 - Introduce class logstash

Issue 2864 - Introduce class logstash

[Felix Dahlke, 2015-10-07 10:46:33]

Looks good, just a few smaller things.

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
File modules/logstash/manifests/init.pp (right):

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/init.pp:58: location =>
"http://packages.elasticsearch.org/logstash/$version/debian",
Can we use HTTPS?

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/init.pp:93: file {'/usr/local/bin/logstash':
Would a symlink do?

If not: AFAIK it's a sort of convention to not add ".sh" to executable scripts.
We always do that with our Python scripts so they're runnable on Windows
(although many of them never need to run there), but I guess there's no reason
to do that with a shell script. No strong opinion however, just a remark.

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
File modules/logstash/manifests/pipeline.pp (right):

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/pipeline.pp:29: # on howto configure a Logstash
pipeline are provided by Elastic at
Nit: "how to"

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/pipeline.pp:47: # For more information on howto use
logstash::fragment with a pipeline
Nit: "how to"

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/pipeline.pp:71: undef => sprintf("# %s\n\n",
join("\n# ", split($description, "\n"))),
Why do we add anything if there's neither content or source? Do we need to add
_something_?

[mathias, 2015-10-20 13:03:34]

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
File modules/logstash/manifests/init.pp (right):

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/init.pp:58: location =>
"http://packages.elasticsearch.org/logstash/$version/debian",
On 2015/10/07 10:46:32, Felix Dahlke wrote:
> Can we use HTTPS?

Yes we can.

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/init.pp:93: file {'/usr/local/bin/logstash':
On 2015/10/07 10:46:32, Felix Dahlke wrote:
> Would a symlink do?
> 
> If not: AFAIK it's a sort of convention to not add ".sh" to executable
scripts.
> We always do that with our Python scripts so they're runnable on Windows
> (although many of them never need to run there), but I guess there's no reason
> to do that with a shell script. No strong opinion however, just a remark.

No, a symbolic link won't do. The /opt/logstash/bin/logstash script under the
hood uses the path of $0 as-is, in order to determine the location of additional
resources.

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
File modules/logstash/manifests/pipeline.pp (right):

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/pipeline.pp:29: # on howto configure a Logstash
pipeline are provided by Elastic at
On 2015/10/07 10:46:32, Felix Dahlke wrote:
> Nit: "how to"

Done.

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/pipeline.pp:47: # For more information on howto use
logstash::fragment with a pipeline
On 2015/10/07 10:46:32, Felix Dahlke wrote:
> Nit: "how to"

Done.

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/pipeline.pp:71: undef => sprintf("# %s\n\n",
join("\n# ", split($description, "\n"))),
On 2015/10/07 10:46:33, Felix Dahlke wrote:
> Why do we add anything if there's neither content or source? Do we need to add
> _something_?

One must supply either $content or $source for concat::fragment, unless $ensure
is "absent" that is.

This particular approach here was meant as an easy way to setup a header for a
pipeline configuration that is spread over multiple fragments. When introduced,
logstash::pipeline did not feature $content and $source yet. However, in it's
current form this now seems rather obsolete. One can achieve pretty much the
same by using $content. I'll remove it.

[mathias, 2015-10-20 13:04:57]

Issue 2864 - Fix grammar nit in logstash::pipeline documentation
Issue 2864 - Remove $logstash::pipeline::description parameter
Issue 2864 - Fix {howto => how to} typo
Issue 2864 - Use HTTPS to download logstash packages

[Felix Dahlke, 2015-10-30 09:43:10]

LGTM

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
File modules/logstash/manifests/init.pp (right):

https://codereview.adblockplus.org/29327588/diff/29327589/modules/logstash/ma...
modules/logstash/manifests/init.pp:93: file {'/usr/local/bin/logstash':
On 2015/10/20 13:03:33, mathias wrote:
> On 2015/10/07 10:46:32, Felix Dahlke wrote:
> > Would a symlink do?
> > 
> > If not: AFAIK it's a sort of convention to not add ".sh" to executable
> scripts.
> > We always do that with our Python scripts so they're runnable on Windows
> > (although many of them never need to run there), but I guess there's no
reason
> > to do that with a shell script. No strong opinion however, just a remark.
> 
> No, a symbolic link won't do. The /opt/logstash/bin/logstash script under the
> hood uses the path of $0 as-is, in order to determine the location of
additional
> resources.

Acknowledged.