Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

Use Apt module for Keys #29

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

Use Apt module for Keys #29

wants to merge 2 commits into from

Conversation

jmkeyes
Copy link
Contributor

@jmkeyes jmkeyes commented Jun 23, 2015

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
Copy link
Contributor

bernd commented Jun 23, 2015

Thanks for the PR!

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

@jmkeyes
Copy link
Contributor Author

jmkeyes commented Jun 24, 2015

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:

  • It used an Exec resource to run apt-get install instead of a Package resource.
  • It required a whitespace separated list of packages and not an array of strings.
  • It had a deprecation notice and a recommendation that it shouldn't be used.

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
Copy link
Contributor

bernd commented Jun 24, 2015

@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
Copy link
Contributor

wjaede commented Jun 24, 2015

@bernd apt-transport-https
I can't remember which combinations ... but that will fix the problem
wjaede@b75d91d

@fungusakafungus
Copy link
Contributor

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/graylog2-puppet@b75d91d change, we changed the above line to Exec['apt_update'] -> Package <| title != 'apt-transport-https' |> and it was good enough.

@jmkeyes
Copy link
Contributor Author

jmkeyes commented Aug 7, 2015

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
Copy link
Contributor

@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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants