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

wants to merge 1 commit into from

Conversation

yachub
Copy link
Contributor

@yachub yachub commented Oct 23, 2021

Pull Request (PR) description

My apologies on missing this. In testing #254 I had manually enable the python36 module stream at some point before hand, but after running on a fresh install I realized certbot requires this module stream to be enabled per:

Error: Execution of '/bin/dnf -d 0 -e 1 -y install certbot' returned 1: Error:
 Problem: package certbot-1.20.0-1.el8.noarch requires python3-certbot = 1.20.0-1.el8, but none of the providers can be installed
  - package python3-certbot-1.20.0-1.el8.noarch requires /usr/bin/python3.6, but none of the providers can be installed
  - conflicting requests
  - package python36-3.6.8-2.module_el8.3.0+6191+6b4b10ec.x86_64 is filtered out by modular filtering
Error: /Stage[main]/Letsencrypt::Install/Package[letsencrypt]/ensure: change from 'purged' to 'present' failed: Execution of '/bin/dnf -d 0 -e 1 -y install certbot' returned 1: Error:
 Problem: package certbot-1.20.0-1.el8.noarch requires python3-certbot = 1.20.0-1.el8, but none of the providers can be installed
  - package python3-certbot-1.20.0-1.el8.noarch requires /usr/bin/python3.6, but none of the providers can be installed
  - conflicting requests
  - package python36-3.6.8-2.module_el8.3.0+6191+6b4b10ec.x86_64 is filtered out by modular filtering

This Pull Request (PR) fixes the following issues

Fixes #236

@yachub yachub force-pushed the el8_enable_stream branch from f5e24f4 to 8dfafe1 Compare October 23, 2021 17:33
@yachub yachub marked this pull request as ready for review October 23, 2021 18:08
bastelfreak
bastelfreak reviewed Oct 25, 2021
View reviewed changes
manifests/init.pp Outdated
@@ -57,6 +58,7 @@
Array $environment = [],
String $package_name = 'certbot',
$package_ensure = 'installed',
String $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.

I think we don't need to allow empty strings:

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

bastelfreak
bastelfreak reviewed Oct 25, 2021
View reviewed changes
manifests/install.pp Outdated
#
class letsencrypt::install (
Boolean $configure_epel = $letsencrypt::configure_epel,
String $package_name = $letsencrypt::package_name,
String $package_ensure = $letsencrypt::package_ensure,
String $dnfmodule_version = $letsencrypt::dnfmodule_version,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
String $dnfmodule_version = $letsencrypt::dnfmodule_version,
String[1] $dnfmodule_version = $letsencrypt::dnfmodule_version,

ekohl
ekohl reviewed Oct 25, 2021
View reviewed changes
manifests/install.pp
) {
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().

@yachub yachub force-pushed the el8_enable_stream branch from 8dfafe1 to 6646398 Compare October 26, 2021 01:59
@ekohl
Copy link
Member

ekohl commented Apr 20, 2022

There is now a merge conflict. Any plans to finish this?

@yachub yachub force-pushed the el8_enable_stream branch from 6646398 to f4206b1 Compare October 1, 2022 12:08
@yachub
(voxpupuli#236) Enable python module stream on EL8. Bump minimum pupp…
b28f212 
…et version

The certbot package on EL8 currently requires `/usr/bin/python3.6`, which is part of the `python36` module stream, and needs enabled in order to install.

Bump the minimum puppet version to 6.15.0 since the enable_only feature of the dnfmodule provider was added in https://tickets.puppetlabs.com/browse/PUP-10235
@yachub yachub force-pushed the el8_enable_stream branch from f4206b1 to b28f212 Compare October 1, 2022 12:11
ekohl
ekohl requested changes Oct 1, 2022
View reviewed changes
manifests/init.pp
@@ -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,

@yachub
Copy link
Contributor Author

yachub commented Oct 1, 2022

There is now a merge conflict. Any plans to finish this?

@ekohl You are really on top of it :) I rebased to see what had changed, but to be honest I'm not sure if this is really of value anymore. I'm currently using the module on a RHEL 9 host now, and I don't think that I remember having to manually enable any streams ¯_(ツ)_/¯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak left review comments

@ekohl ekohl ekohl requested changes

Assignees
No one assigned
Labels
merge-conflicts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RHEL8 support for dns-rfc2136
3 participants
@yachub @ekohl @bastelfreak