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

Allow to set install_options for the Package resource #28

Closed
wants to merge 2 commits into from

Conversation

kwisatz
Copy link
Contributor

@kwisatz kwisatz commented Dec 19, 2016

Again, this is more a basis for discussion than a real PR I would see merged in as-is.

The Package resource allows for "install_options" to be set, which is useful on Debian $::osfamily for instance to set --no-install-recommends (Specifically in order to avoid installing a vulnerable version of samba these days together with the icinga2 client.)

Generally though, but I'm not sure Resource defaults have an impact on stdlib functions (scope?), maybe we should use ensure_packages() rather than Package directly in order to allow for people to declare some packages themselves without running into duplicate resource declaration issues?

Fixes the typo in both the README and the manifest documentation
sections. Also automatically removed whitespace in README.de and the
manifest file.
@bobapple
Copy link
Member

I like the simplicity of the module and I would prefer the solution with ensure_packages over adding another parameter for install options. However, as you already mentioned, we would need to check if resource defaults still work with ensure_pacakges, as we use them for windows to set chocolatey as package provider.

On the other hand, even with ensure_packages Puppet will detect duplicates if the user defines a Package resource with the same name. The user would be required to use ensure_packages as well. But again, its better then nothing.

On a side note, samba is being installed automatically when installing Icinga 2 on Debian? I'm just wondering I was not able to reproduce this.

@kwisatz
Copy link
Contributor Author

kwisatz commented Dec 22, 2016

I like the simplicity of the module and I would prefer the solution with ensure_packages over adding another parameter for install options. However, as you already mentioned, we would need to check if resource defaults still work with ensure_packages, as we use them for windows to set chocolatey as package provider.

I could give it a try, but as I mention below, it won't solve the duplicate resource issue. However, as you rightly mention at the end, it's not icinga2 pulling in smbclient, so using ensure_packages() could be enough to make this work in must situations.

On the other hand, even with ensure_packages Puppet will detect duplicates if the user defines a Package resource with the same name. The user would be required to use ensure_packages as well. But again, its better then nothing.

Yes, and even then, that doesn't solve duplication issues whenever the parameters passed differ from the original ones. ensure_packages will only silently discard the package call if it has exactly the same signature.

So, assuming the icinga module would use ensure_packages([]) my using ensure_packages([], { 'install_options' => []}) would still create a duplicate resource declaration.

I feel like this is either a real shortcoming in either Puppet or my understanding of it.

On a side note, samba is being installed automatically when installing Icinga 2 on Debian? I'm just wondering I was not able to reproduce this.

No, sorry, I just realized it's not icinga2 pulling in smbclient, but the standard plugins package. My bad.

i monitoring-plugins-standard Recommends smbclient                              
pB  smbclient                   Depends    samba-common (= 2:4.2.14+dfsg-0+deb8u2)

@bobapple
Copy link
Member

So, assuming the icinga module would use ensure_packages([]) my using ensure_packages([], { 'install_options' => []}) would still create a duplicate resource declaration.

This shouldn't create a duplicate entry. It would only create a duplicate if we use ensure_packages in the module and the user uses the package resource directly (without ensure_packages). So the user must know that he has to use ensure_packages as well.

@kwisatz
Copy link
Contributor Author

kwisatz commented Dec 27, 2016

But it will create a duplicate entry nonetheless. I remember having read so in Puppet docs somewhere, but I can't find it right now.
In any case, I tried it again, just to be sure and am getting a duplicate declaration.

E.g. changing code in install.pp:

+ ensure_packages([ $package, ], { ensure => installed })
-   package { $package:
-   ensure => installed,
- }

And adding another icinga2 package to e.g. my icinga2/server.pp manifest (with diverging defaults:)

ensure_packages([
        'icinga2',
        'monitoring-plugins-standard',
        'nagios-plugins-contrib',
        'libmonitoring-plugin-perl',
    ], { install_options => ['--no-install-recommends']})

Will result in:

Error: Could not retrieve catalog from remote server: Error 400 on SERVER: Evaluation Error: Error while evaluating a Function Call, Duplicate declaration: Package[icinga2] is already declared; cannot redeclare at /etc/puppet/environments/cl1_prod/modules/icinga2/manifests/install.pp:33:3 on node icinga.int.mde-content.com
Warning: Not using cache on failed catalog
Error: Could not retrieve catalog; skipping run

If I were to use the same signature both times, there would indeed not be any resource duplication.

@bobapple
Copy link
Member

Yeah, there would also be no chance for ensure_package to know which one of the declarations is the right one. Somehow I thought this could work, but I think we can dismiss this as a solution.

Adding a parameter doesn't work as well. We support multiple operating systems, adding custom options would break the installation process.

@kwisatz
Copy link
Contributor Author

kwisatz commented Dec 28, 2016

Adding a parameter doesn't work as well. We support multiple operating systems, adding custom options would break the installation process.

It's not (that) relevant anymore since we determined that the culprit is not icinga2 but the monitoring-plugins-standard package, but that I don't understand.
My idea was to let the user handle the options passed to the module, not to hardcode, but to allow for it. It would be their responsibility if they decided to pass an option to a Windows machine that it doesn't understand.

@kwisatz kwisatz closed this Dec 28, 2016
@zachfi
Copy link
Contributor

zachfi commented Dec 29, 2016

@kwisatz Coming in a bit late, but I've got the following for my debian boxes:

  apt::conf { 'norecommends':
    priority => '00',
    content  => "Apt::Install-Recommends 0;\nApt::AutoRemove::InstallRecommends 1;\n",
  }

This uses the puppetlabs-apt module to do functionally what you are after, but on a system wide scale. It might be worth considering. Maybe it helps.

@kwisatz
Copy link
Contributor Author

kwisatz commented Dec 29, 2016

Uh… thanks, that's a good idea. Guess I got kind of stuck in my narrow-minded approach.

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.

None yet

3 participants