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

Add tests for gentoo #86

Merged
merged 1 commit into from
Jul 13, 2020
Merged

Add tests for gentoo #86

merged 1 commit into from
Jul 13, 2020

Conversation

chrekh
Copy link
Contributor

@chrekh chrekh commented Jul 12, 2020

No description provided.

Comment on lines 46 to 64
['0.pool.ntp.org', '1.pool.ntp.org', '2.pool.ntp.org', '3.pool.ntp.org'].each do |s|
it { is_expected.to contain_file('/etc/chrony/chrony.conf').with_content(%r{^\s*server #{s} iburst$}) }
end
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid multiple it statements:

Suggested change
['0.pool.ntp.org', '1.pool.ntp.org', '2.pool.ntp.org', '3.pool.ntp.org'].each do |s|
it { is_expected.to contain_file('/etc/chrony/chrony.conf').with_content(%r{^\s*server #{s} iburst$}) }
end
it do
is_expected.to contain_file('/etc/chrony/chrony.conf')
.with_content(%r{^\s*server 0.pool.ntp.org iburst$})
.with_content(%r{^\s*server 1.pool.ntp.org iburst$})
.with_content(%r{^\s*server 3.pool.ntp.org iburst$})
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. But then that should be fixed for the other osfamily also. If you like, I can add a commit for that in this PR?

Comment on lines 53 to 57
it { is_expected.to contain_file('/etc/chrony/chrony.keys').with_mode('0644') }
it { is_expected.to contain_file('/etc/chrony/chrony.keys').with_owner('0') }
it { is_expected.to contain_file('/etc/chrony/chrony.keys').with_group('0') }
it { is_expected.to contain_file('/etc/chrony/chrony.keys').with_replace(true) }
it { is_expected.to contain_file('/etc/chrony/chrony.keys').with_content("0 xyzzy\n") }
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Multiple it statements require the catalog to be compiled multiple times and is much slower in my experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reply here, it should be fixed for all osfamily.

Copy link
Contributor

Choose a reason for hiding this comment

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

my suggestion:
fix for Gentoo as suggested by @ekohl, fix for other OSes in new pr
add a TODO comment, if you're the forgetful type ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll do that.

@chrekh
Copy link
Contributor Author

chrekh commented Jul 12, 2020

I have updated the PR with the suggested changes. I agree, this looks so much better. I'll submit a PR for the other osfamily later.

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!

Edit: tests are still running. Other than that this can be merged.

@chrekh
Copy link
Contributor Author

chrekh commented Jul 13, 2020

And thank you for all good reviews, not only are we improving the code, but I'm learning a lot and having fun doing it.

@ekohl ekohl merged commit 697f38f into voxpupuli:master Jul 13, 2020
@chrekh chrekh deleted the gentootests branch July 13, 2020 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants