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

Add support for CentOS 8, Debian 10 #115

Closed
wants to merge 4 commits into from
Closed

Add support for CentOS 8, Debian 10 #115

wants to merge 4 commits into from

Conversation

dhoppe
Copy link
Member

@dhoppe dhoppe commented Apr 27, 2020

No description provided.

@dhoppe dhoppe added enhancement New feature or request tests-fail needs-work not ready to merge just yet labels Apr 27, 2020
@dhoppe
Copy link
Member Author

dhoppe commented Apr 27, 2020

The acceptance tests need to be refactored, because they are written for Vagrant.

@ekohl
Copy link
Member

ekohl commented Apr 28, 2020

You can also ensure the user before hand:

before do
apply_manifest(wget_manifest, catch_failures: true)
end

That part runs as root and can create the user if needed. Should be the easiest fix.

@dhoppe
Copy link
Member Author

dhoppe commented Apr 28, 2020

This module is broken in so many ways and I do not know, why it has not been archived in favour of puppet/archive.

Even if I remove the $strict variables, the acceptance tests will fail, because the user Vagrant does not have sudo permissions to install wget, if necessary. For example at CentOS hosts.

package { 'wget': ensure => $version }
}
elsif $::kernel == 'FreeBSD' {
elsif $facts['kernel'] == 'FreeBSD' {
if versioncmp($facts['os']['release']['major'], '10') >= 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only supported version, so you can simplify this whole block.

@@ -1,7 +1,7 @@
require 'spec_helper_acceptance'

describe 'wget' do
let(:wget_manifest) { "class { 'wget': }" }
let(:wget_manifest) { "user { 'vagrant': ensure => present, managehome => true } -> class { 'wget': }" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep this local to the user test since it's not needed as part of the other tests.

@Dan33l
Copy link
Member

Dan33l commented Apr 28, 2020

why we consume energy to maintain a deprecated module ?

@Dan33l
Copy link
Member

Dan33l commented Apr 28, 2020

IMO we should set this module in archive mode.

@alexjfisher
Copy link
Member

IMO we should set this module in archive mode.

It's a dependency of some of our other modules though??? Fix up those first?

@alexjfisher
Copy link
Member

IMO we should set this module in archive mode.

It's a dependency of some of our other modules though??? Fix up those first?

Maybe not ours... I have puppet/wget in my Puppetfile because I used a nexus module. But maybe we should adopt that? hubspotdevops/puppet-nexus#116 (comment)

@ekohl
Copy link
Member

ekohl commented Nov 27, 2020

Going to close this since it's deprecated. See #119.

@ekohl ekohl closed this Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-work not ready to merge just yet tests-fail
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants