-
Notifications
You must be signed in to change notification settings - Fork 271
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
Pre-install foreman-selinux in acceptance environment #1041
base: master
Are you sure you want to change the base?
Conversation
require => Class['foreman::repo'], | ||
} | ||
|
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.
} |
require => Class['foreman::repo'], | ||
} | ||
|
||
if $foreman::repo::configure_scl_repo { |
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.
if $foreman::repo::configure_scl_repo { | |
if $foreman::repo::configure_scl_repo { |
ensure => installed, | ||
require => Class['foreman::repo'], | ||
} | ||
} |
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.
This whole block is indented too far I think. And you'll have a trailing }
on line 18.
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.
It was my intention to have the rh-redis5-redis
package inside if $facts['os']['selinux']['enabled'] { ... }
because of the existing comment on L#5: # Needed for idempotency when SELinux is enabled
(which now conveniently applies to both package resources)
IMO it was an oversight that this wasn't already conditioned on selinux being enabled.
I thought about making the change in a separate commit but it seemed too minor to warrant it. That would have made this easier to review but probably the git log clutter is not worth it when this only affects tests.
What do you think?
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.
That works. Can you rebase on latest master branch to ensure tests pass?
c87c467
to
c7fbee0
Compare
Hey @ehelms , I triggered the tests to run again. The failures on Ubuntu look non-related:
|
Yeah, ubuntu 20.04 tests are busted. |
Also do not purge foreman-selinux between tests. This is a workaround for the Puppet issue [PUP-10548].
c7fbee0
to
f340ecf
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.
Needs a rebase now.
@@ -1,7 +1,7 @@ | |||
def purge_foreman | |||
case fact('osfamily') | |||
when 'RedHat' | |||
on default, 'yum -y remove foreman* tfm-*' |
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.
Isn't there a way to exclude a package in yum/dnf remove?
Also do not purge foreman-selinux between tests. This is a workaround
for the Puppet issue [PUP-10548].