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

Add support for AppStream package installation #1459

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

Add support for AppStream package installation #1459

wants to merge 2 commits into from

Conversation

Henrik-Hansson
Copy link

Pull Request (PR) description

Add support to install nginx through AppStream on EL8.

This Pull Request (PR) fixes the following issues

@puppet-community-rangefinder
Copy link

nginx is a class

Breaking changes to this file WILL impact these 15 modules (exact match):
Breaking changes to this file MAY impact these 34 modules (near match):

nginx::package::redhat is a class

that may have no external impact to Forge modules.

This module is declared in 9 of 577 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

smortex
smortex reviewed Jul 20, 2021
View reviewed changes
manifests/init.pp Outdated Show resolved Hide resolved
manifests/package/redhat.pp Outdated Show resolved Hide resolved
@anders-larsson
Copy link

It seems this pull request needs additional work. Currently it attempts to install the "package" every time Puppet is executed.

ekohl
ekohl reviewed Aug 19, 2021
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Maybe it helps, but in our Candlepin module we use enable_only => true: https://github.com/theforeman/puppet-candlepin/blob/27a0f4d4de5d2180309db05a508041d997139739/manifests/install.pp#L14-L21

I'm not sure if that can also switch modules and haven't looked at the details if I'm honest.

manifests/package/redhat.pp Outdated Show resolved Hide resolved
ekohl
ekohl approved these changes Mar 13, 2022
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let me know if you want to address my comment. Without it it's already a good change IMHO, but it can help other RHEL clones.

manifests/package/redhat.pp Outdated
@@ -94,6 +95,15 @@
}
}

if $dnfmodule and fact('os.name') in ['RedHat', 'CentOS', 'VirtuozzoLinux'] and versioncmp(fact('os.release.major'), '8') >= 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to use os.family instead?

@Henrik-Hansson
Copy link
Author

I agree that os.family seems better. I will rebase and update with os.family.

ekohl
ekohl approved these changes Mar 14, 2022
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks!

@ekohl
Copy link
Member

ekohl commented Mar 14, 2022

I think #1501 should fix the acceptance tests, except for EL7. But that shouldn't hold up this PR. Unit tests should suffice for that (but those are still running).

@Henrik-Hansson Henrik-Hansson force-pushed the feature_dnfmodule branch 2 times, most recently from a5c142f to 632ad83 Compare November 16, 2022 12:36
@Henrik-Hansson
Copy link
Author

Hello, what can I do to get this merged?
The CI validation fails on something not related to this PR.
BR,
Henrik

@ekohl
Copy link
Member

ekohl commented Nov 28, 2022

I've opened #1519 to fix that.

@ekohl
Copy link
Member

ekohl commented Nov 28, 2022

Please rebase.

@Henrik-Hansson Henrik-Hansson force-pushed the feature_dnfmodule branch 2 times, most recently from fe27992 to 9020d4f Compare December 19, 2022 17:56
@Henrik-Hansson
Copy link
Author

Hi, sorry for delay.
Most of the tests fails on passenger, don't think it's related to my change.
I have fixed the Package error I introduced with "Package['nginx']", with "'Package[nginx]'".

kenyon
kenyon requested changes Dec 19, 2022
View reviewed changes
manifests/init.pp Show resolved Hide resolved
@ardrigh
Copy link
Contributor

ardrigh commented Aug 26, 2023

@kenyon is it possible for you to do a fresh review for this to get merged?

It appears the documentation has been added, and this is much needed improvement for RHEL servers.

@kenyon kenyon force-pushed the feature_dnfmodule branch from 9312c24 to 3aad2f2 Compare August 26, 2023 15:02
@kenyon
Copy link
Member

kenyon commented Aug 26, 2023

#1573 removes EOL Ubuntu 18.04.

@ardrigh
Copy link
Contributor

ardrigh commented Aug 27, 2023

@Henrik-Hansson if you are still keen to update this pull request, the version check could use a tweak.

RHEL9 started with 1.20, a version check for this should fix the tests.

It might be easier to assume a test for nginx 1.22 that is available in both RHEL8.8 and RHEL9.2 appstreams?

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

@smortex smortex smortex left review comments

@ekohl ekohl ekohl approved these changes

@kenyon kenyon kenyon approved these changes

Assignees
No one assigned
Labels
tests-fail
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Henrik-Hansson @anders-larsson @ekohl @ardrigh @kenyon @smortex