graylog-labs / graylog2-puppet

[DEPRECATED] Puppet module to install and manage a Graylog 1.x system.
https://www.graylog.org/
MIT License
10 stars 28 forks source link

Use Apt module for Keys #29

Open jmkeyes opened 9 years ago

jmkeyes commented 9 years ago

This PR replaces the hard-coded PGP key with one pulled dynamically from the pgp.surfnet.nl key server. It also removes the required_packages parameter from the apt::source to avoid an obscure (and somewhat hard to diagnose) error on Puppet 4.1.

bernd commented 9 years ago

Thanks for the PR!

Using required_packages was used to fix #15. Any idea why this does not work in 4.1?

jmkeyes commented 9 years ago

If I remember correctly, it was:

undefined method `key_attributes' for nil:NilClass

When I encountered the error, I was trying to apply the ::graylog2::repo class to a Debian 7 server.

I removed the ::graylog2::repo class and substituted the resources it contained into the manifest instead, repeating the process and "bisecting" the application of resources and their parameters until I could get the catalog to compile by removing the required_packages parameter.

After looking at the documentation of apt::source for the required_packages parameter I found a number of concerns:

I expect that the specific error I encountered was probably due to the way Puppet 4.1 handles array parameters, but it nevertheless seemed that using a require was more descriptive of it's intent.

Regarding #15, I'd like to see what combination of modules and manifests triggered a dependency cycle to ensure it doesn't happen in my environment as well.

Cheers!

bernd commented 9 years ago

@jmkeyes Thanks for the detailed feedback!

@wjaede Can you remember which combination of modules and manifests triggered the dependency cycle you reported in #15? Thank you!

wjaede commented 9 years ago

@bernd apt-transport-https I can't remember which combinations ... but that will fix the problem https://github.com/wjaede/graylog2-puppet/commit/b75d91da94dd05ed26d49887bb1d0b9f7dc9cce0

fungusakafungus commented 9 years ago

wrt dependency cycles:

We used to have this defined in a common manifest:

Exec['apt_update'] -> Package <| |>

(Exec['apt_update'] is part of apt::source)

As we started to use this puppet module before the wjaede@b75d91d change, we changed the above line to Exec['apt_update'] -> Package <| title != 'apt-transport-https' |> and it was good enough.

jmkeyes commented 9 years ago

I haven't been able to reproduce the dependency cycle in any manifest so far.

Let me know if you'd like any changes to this PR.

Thanks!

jalogisch commented 6 years ago

@jmkeyes could you please sign the CLA and check if this PR might need some <3 before it can be merged from your end?

If we did not get feedback within a week i'll close the PR.

Thank you