-
-
Notifications
You must be signed in to change notification settings - Fork 881
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 AppStream package installation #1459
base: master
Are you sure you want to change the base?
Conversation
dd6797d
to
e5651cb
Compare
It seems this pull request needs additional work. Currently it attempts to install the "package" every time Puppet is executed. |
There was a problem hiding this 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.
e66c3ed
to
b0d8ba2
Compare
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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?
I agree that os.family seems better. I will rebase and update with os.family. |
b0d8ba2
to
7a759a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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). |
a5c142f
to
632ad83
Compare
Hello, what can I do to get this merged? |
I've opened #1519 to fix that. |
Please rebase. |
fe27992
to
9020d4f
Compare
Hi, sorry for delay. |
9020d4f
to
9312c24
Compare
@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. |
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
9312c24
to
3aad2f2
Compare
#1573 removes EOL Ubuntu 18.04. |
@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? |
Pull Request (PR) description
Add support to install nginx through AppStream on EL8.
This Pull Request (PR) fixes the following issues