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.

Sign up for GitHub

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

Jump to bottom

(#236) Enable python module stream on EL8 #255

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

(#236) Enable python module stream on EL8 #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions REFERENCE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ The following parameters are available in the `letsencrypt` class:
* [`environment`](#environment)
* [`package_name`](#package_name)
* [`package_ensure`](#package_ensure)
* [`dnfmodule_version`](#dnfmodule_version)
* [`package_command`](#package_command)
* [`config_file`](#config_file)
* [`config`](#config)
Expand Down Expand Up @@ -119,6 +120,14 @@ The value passed to `ensure` when installing the client package.

Default value: `'installed'`

##### <a name="dnfmodule_version"></a>`dnfmodule_version`

Data type: `String`

The yum module stream version to enable on EL8 and greater variants using the `package` method with the `dnfmodule` provider.

Default value: `'python36'`

##### <a name="package_command"></a>`package_command`

Data type: `String`
Expand Down Expand Up @@ -305,6 +314,7 @@ The following parameters are available in the `letsencrypt::install` class:
* [`configure_epel`](#configure_epel)
* [`package_ensure`](#package_ensure)
* [`package_name`](#package_name)
* [`dnfmodule_version`](#dnfmodule_version)

##### <a name="configure_epel"></a>`configure_epel`

Expand All @@ -330,6 +340,14 @@ Name of package to use when installing the client package.

Default value: `$letsencrypt::package_name`

##### <a name="dnfmodule_version"></a>`dnfmodule_version`

Data type: `String`

The yum module stream version to enable on EL8 and greater variants using the `package` method with the `dnfmodule` provider.

Default value: `$letsencrypt::dnfmodule_version`

### <a name="letsencryptplugindns_cloudflare"></a>`letsencrypt::plugin::dns_cloudflare`

This class installs and configures the Let's Encrypt dns-cloudflare plugin.
Expand Down
2 changes: 2 additions & 0 deletions manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
# @param environment An optional array of environment variables
# @param package_name Name of package and command to use when installing the client package.
# @param package_ensure The value passed to `ensure` when installing the client package.
# @param dnfmodule_version The yum module stream version to enable on EL8 and greater variants using the `package` method with the `dnfmodule` provider.
# @param package_command Path or name for letsencrypt executable.
# @param config_file The path to the configuration file for the letsencrypt cli.
# @param config A hash representation of the letsencrypt configuration file.
Expand Down Expand Up @@ -57,6 +58,7 @@
Array $environment = [],
String $package_name = 'certbot',
$package_ensure = 'installed',
String[1] $dnfmodule_version = 'python36',
Copy link
Member

Choose a reason for hiding this comment

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

You can't set this to python36 since it's only valid for EL8 but it's invalid for anything else. It would be best to create an 8.yaml here with python36 and then set it to ~ in common.yaml. Then change it to

Suggested change
String[1] $dnfmodule_version = 'python36',
Optional[String[1]] $dnfmodule_version,

String $package_command = 'certbot',
Stdlib::Unixpath $config_dir = '/etc/letsencrypt',
String $config_file = "${config_dir}/cli.ini",
Expand Down
11 changes: 11 additions & 0 deletions manifests/install.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,23 @@
# @param configure_epel A feature flag to include the 'epel' class and depend on it for package installation.
# @param package_ensure The value passed to `ensure` when installing the client package.
# @param package_name Name of package to use when installing the client package.
# @param dnfmodule_version The yum module stream version to enable on EL8 and greater variants using the `package` method with the `dnfmodule` provider.
#
class letsencrypt::install (
Boolean $configure_epel = $letsencrypt::configure_epel,
String $package_name = $letsencrypt::package_name,
String $package_ensure = $letsencrypt::package_ensure,
String[1] $dnfmodule_version = $letsencrypt::dnfmodule_version,
) {
if ($facts['os']['family'] == 'RedHat' and $facts['os']['release']['major'] >= '8') {
package { 'enable python module stream':
Copy link
Member

Choose a reason for hiding this comment

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

It surprises me this is needed since they are all default modules which AFAIK means they are automatically enabled if needed.

If this is needed, it needs a before => Package['letsencrypt'] for proper ordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I spun up a fresh AlmaLinux 8 vagrant box to test. Each of those streams are set to [d] (default), but not enabled [e] by default, however they do become automatically enabled if a package requires a dependency in that stream. Unless a user happens to disables it before using the module via dnf module disable python36, then that's what results in the failure above.

So this remediates errors in that scenario by ensuring that the stream is enabled first. To me it seems similar to ensuring that the epel repo (with configure_epel) is enabled before installing packages. Do you think there should be a parameter like configure_modulestream to control that behavior? Maybe I'm overthinking it.

Copy link
Member

Choose a reason for hiding this comment

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

No, I think that's a fair point that I hadn't considered yet. I do think it can make sense to introduce a class parameter Boolean $manage_python_module_stream = $facts['os']['family'] == 'RedHat' and $facts['os']['release']['major'] >= '8'.

I also wonder if this comparison works well. For example, you can break Fedora support where AFAIK certbot is not built modular. And I'm hesitant to compare strings as if they were versions. Is '10' >= '8'? Generally I always use versioncmp().

name => $dnfmodule_version,
enable_only => true,
provider => 'dnfmodule',
ekohl marked this conversation as resolved.
Show resolved Hide resolved
before => Package['letsencrypt'],
}
}

package { 'letsencrypt':
ensure => $package_ensure,
name => $package_name,
Expand Down
2 changes: 1 addition & 1 deletion metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
"requirements": [
{
"name": "puppet",
"version_requirement": ">= 6.1.0 < 8.0.0"
"version_requirement": ">= 6.15.0 < 8.0.0"
}
],
"dependencies": [
Expand Down
10 changes: 9 additions & 1 deletion spec/classes/letsencrypt_install_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
{
configure_epel: false,
package_ensure: 'installed',
package_name: 'letsencrypt'
package_name: 'letsencrypt',
dnfmodule_version: 'python36'
}
end
let(:additional_params) { {} }
Expand Down Expand Up @@ -44,6 +45,13 @@
is_expected.to contain_package('letsencrypt').that_requires('Class[epel]')
end
end

case facts[:operatingsystemmajrelease]
when '7'
it { is_expected.not_to contain_package('enable python module stream').with_name('python36') }
when '8'
it { is_expected.to contain_package('enable python module stream').with_name('python36').with_enable_only('true').with_provider('dnfmodule') }
end
end
end
end
Expand Down