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.

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

Update defaults to match RHEL8/9 #195

Merged
merged 1 commit into from
Jul 13, 2022
Merged

Update defaults to match RHEL8/9 #195

merged 1 commit into from
Jul 13, 2022

Conversation

jcpunk
Copy link

@jcpunk jcpunk commented Jun 2, 2022

Pull Request (PR) description

RHEL8 and later use a slightly different default for wtmp

This Pull Request (PR) fixes the following issues

N/A

@@ -717,7 +717,7 @@
end
let(:facts) do
{
os: { family: 'RedHat' },
os: { family: 'RedHat', release: { major: '7' } },
Copy link
Member

Choose a reason for hiding this comment

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

we should really port this to rspec-puppet-facts soonish

@bastelfreak
Copy link
Member

Thanks for the PR. Can you please add CentOS 8/9 to metadata.json?

@jcpunk
Copy link
Author

jcpunk commented Jun 3, 2022

Done

@jcpunk jcpunk requested a review from bastelfreak June 3, 2022 13:37
@bastelfreak
Copy link
Member

can you please take a look at the failing tests?

@jcpunk
Copy link
Author

jcpunk commented Jun 3, 2022

I have no idea why these tests are failing.... there is something amiss with the OS detection code but that is beyond me....

Copy link

@vchepkov vchepkov left a comment

Choose a reason for hiding this comment

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

Seems complicated, how about

$wtmp_missingok = versioncmp($facts['os']['release']['major'],'7')) <= 0 ? {
   true => false,
  default => true,
}
and no need to create duplicate resource definition?

@jcpunk
Copy link
Author

jcpunk commented Jun 6, 2022

Updated with the suggested form.

@jcpunk jcpunk requested a review from vchepkov June 6, 2022 16:37
@jcpunk
Copy link
Author

jcpunk commented Jun 6, 2022

I honestly don't see how these changes are suddenly making the self tests fail the group ownership or file ensures....

Copy link

@vchepkov vchepkov left a comment

Choose a reason for hiding this comment

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

LGTM

@jcpunk
Copy link
Author

jcpunk commented Jul 6, 2022

Rebased following latest module sync

@jcpunk
Copy link
Author

jcpunk commented Jul 6, 2022

The failures I'm showing are part of the existing failure set.

Comment on lines 108 to 111
$wtmp_missingok = versioncmp($facts['os']['release']['major'],'7') <= 0 ? {
true => false,
default => true,
}
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this equivalent?

Suggested change
$wtmp_missingok = versioncmp($facts['os']['release']['major'],'7') <= 0 ? {
true => false,
default => true,
}
$wtmp_missingok = versioncmp($facts['os']['release']['major'],'8') >= 0

Copy link
Author

Choose a reason for hiding this comment

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

done

Comment on lines 66 to 94
if facts[:operatingsystemmajrelease].to_i <= 7
it {
is_expected.to contain_logrotate__rule('wtmp').with(
'path' => '/var/log/wtmp',
'create_mode' => '0664',
'missingok' => false,
'minsize' => '1M',
'create' => true,
'create_owner' => 'root',
'create_group' => 'utmp',
'rotate' => 1,
'rotate_every' => 'monthly'
)
}
else
it {
is_expected.to contain_logrotate__rule('wtmp').with(
'path' => '/var/log/wtmp',
'create_mode' => '0664',
'missingok' => true,
'minsize' => '1M',
'create' => true,
'create_owner' => 'root',
'create_group' => 'utmp',
'rotate' => 1,
'rotate_every' => 'monthly'
)
}
end
Copy link
Member

Choose a reason for hiding this comment

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

Rather than duplicating it, thoughts on the following (where I didn't bother with correctly indenting):

Suggested change
if facts[:operatingsystemmajrelease].to_i <= 7
it {
is_expected.to contain_logrotate__rule('wtmp').with(
'path' => '/var/log/wtmp',
'create_mode' => '0664',
'missingok' => false,
'minsize' => '1M',
'create' => true,
'create_owner' => 'root',
'create_group' => 'utmp',
'rotate' => 1,
'rotate_every' => 'monthly'
)
}
else
it {
is_expected.to contain_logrotate__rule('wtmp').with(
'path' => '/var/log/wtmp',
'create_mode' => '0664',
'missingok' => true,
'minsize' => '1M',
'create' => true,
'create_owner' => 'root',
'create_group' => 'utmp',
'rotate' => 1,
'rotate_every' => 'monthly'
)
}
end
it do
is_expected.to contain_logrotate__rule('wtmp').with(
'path' => '/var/log/wtmp',
'create_mode' => '0664',
'missingok' => facts[:operatingsystemmajrelease].to_i >= 8,
'minsize' => '1M',
'create' => true,
'create_owner' => 'root',
'create_group' => 'utmp',
'rotate' => 1,
'rotate_every' => 'monthly'
)
end

Copy link
Author

Choose a reason for hiding this comment

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

done

@ekohl ekohl merged commit ae480a5 into voxpupuli:master Jul 13, 2022
@ekohl ekohl added the enhancement New feature or request label Jul 13, 2022
@jcpunk jcpunk deleted the wtmp-rhel9 branch July 13, 2022 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants