Rebase of #683 / mock systemd fact properly #715
Conversation
|
Line 152/159 in spec/classes/rabbitmq_spec.rb simply print nothing?! |
|
This now works as soon as we have a new release of |
bastelfreak
changed the title
bastelfreak
force-pushed
the
wip
branch
4 times, most recently
from
b955458 to
431e65d
Compare
| Boolean $stomp_ssl_only = $rabbitmq::params::stomp_ssl_only, | ||
| Boolean $wipe_db_on_cookie_change = $rabbitmq::params::wipe_db_on_cookie_change, | ||
| String $cluster_partition_handling = $rabbitmq::params::cluster_partition_handling, | ||
| Variant[Integer[-1],Enum['unlimited'],Pattern[/^(infinity|\d+(:(infinity|\d+))?)$/]] $file_limit = $rabbitmq::params::file_limit, |
There was a problem hiding this comment.
This is the reason why I don't like the = alignment ;) I do wonder if puppet-systemd should have a type for this so you can reuse it in cases like these. Currently it's hidden in a struct so no reuse there.
There was a problem hiding this comment.
I think a reuse is possible, if we would use the type correctly.
if $facts['systemd'] { # systemd fact provided by systemd module
systemd::service_limits { 'rabbitmq.service':
limits => {'LimitNOFILE' => $file_limit},
# The service will be notified when config changes
restart_service => false,
}
}We could reuse the struct if we would allow people to pass the whole hash. What we could do: Create a new optional parameter with the struct from puppet-systemd. If it is set we pass it to $systemd::service_limits::limits, otherwise we use $rabbitmq::file_limit. This would be easier if $rabbitmq::file_limit would not be used for legacy /etc/limits entries as well.
There was a problem hiding this comment.
Re: Alignment, it is way easier to read if you ignore whitespace changes in the diff.
| when 'RedHat' | ||
| case facts[:os]['release']['major'] | ||
| when '6' | ||
| { service_provider: 'sysv', systemd: false } |
There was a problem hiding this comment.
Technically EL6 was upstart. Probably won't matter.
spec/spec_helper_local.rb
Outdated
| case facts[:os]['release']['major'] | ||
| when '6' | ||
| { service_provider: 'sysv', systemd: false } | ||
| when '7' |
There was a problem hiding this comment.
Should this just be else? That way you're future proof.
There was a problem hiding this comment.
I still see this as when '7'. I can see arguments for both ways - but in this case, probably makes most sense to have it be else.
spec/spec_helper_local.rb
Outdated
| case facts[:os]['release']['major'] | ||
| when '7' | ||
| { service_provider: 'sysv', systemd: false } | ||
| when '8' |
There was a problem hiding this comment.
You can catch this as else as well.
| add_custom_fact :rabbitmq_version, '3.6.1' # puppet-rabbitmq | ||
| add_custom_fact :erl_ssl_path, '/usr/lib64/erlang/lib/ssl-7.3.3.1/ebin' # puppet-rabbitmq | ||
|
|
||
| def os_specific_facts(facts) |
There was a problem hiding this comment.
Alternative:
add_custom_fact :systemd, lambda do |os, facts|
case facts[:os]['family']
when 'Debian'
case facts[:os]['name']
when 'Debian'
facts[:os]['release']['major'].to_i >= 8
when 'Ubuntu'
facts[:os]['release']['major'].to_i >= 16
else
true
end
when 'RedHat'
facts[:os]['release']['major'].to_i >= 7
else
true
end
endThe benefit is that you don't have to merge it into facts in every test but it's just present. You do need to duplicate it for service_provider if you want that to be present. We don't use that now AFAIK so I'd leave it out. If it's important, you could probably rewrite it to use the service_provider fact:
add_custom_fact :systemd, lambda { |os, facts| facts[:service_provider] == 'systemd' }There was a problem hiding this comment.
service_provider fact comes from stdlib and isn't available in facterdb :( I will update it to your first approach and also propose it to modulesync_config
There was a problem hiding this comment.
for now I would like to stick with the current version and implement something nice for both facts in modulsync_config. I am not sure where os and facts should come from in your code and it give me an error:
ArgumentError:
tried to create Proc object without a block
| end | ||
| end | ||
| end | ||
|
|
||
| [-42, '-42', 'foo', '42'].each do |value| | ||
| [-42, '-42', 'foo'].each do |value| | ||
| context "with file_limit => '#{value}'" do |
There was a problem hiding this comment.
Are positive numbers not supported (either as string or int) here?
There was a problem hiding this comment.
it took me a three times to read this code correctly, but this array lists unsupported values. quoted strings are now allowed.
spec/spec_helper_local.rb
Outdated
| case facts[:os]['release']['major'] | ||
| when '6' | ||
| { service_provider: 'sysv', systemd: false } | ||
| when '7' |
There was a problem hiding this comment.
I still see this as when '7'. I can see arguments for both ways - but in this case, probably makes most sense to have it be else.
I'm trying things out here. #683 itself is working now, with the unreleased changes in puppet-systemd. However I'm running into one strange rspec issue where it seems to not properly mock the systemd/service_provider fact.