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

Cleanups #331

Merged
merged 7 commits into from
Jun 27, 2016
Merged

Cleanups #331

merged 7 commits into from
Jun 27, 2016

Conversation

DavidS
Copy link
Contributor

@DavidS DavidS commented Jun 24, 2016

No description provided.

on_supported_os.select { |_, f| f[:os]['family'] != 'Solaris' }.each do |os, f|
context "on #{os}" do
let(:facts) do
f.merge(super())
Copy link
Contributor

Choose a reason for hiding this comment

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

You've reversed the merge operation here, does that still produce the desired output? In the previous implementation, it merged the os specific facts into super() and now it is merging super() into the os facts. This merge will overwrite any duplicate keys in f with the value in super() with this implementation... even nil values.

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, that's intentional. Prior to this, super had defaults that had the os-specific facts added to it. Using rspec-puppet-facts, f has ALL facts, and super() becomes more specific, as it is specific for the current test group.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Thanks for the explanation.

@@ -20,6 +20,7 @@ group :development, :unit_tests do
gem 'puppetlabs_spec_helper', :require => false
gem 'rspec-puppet', '>= 2.3.2', :require => false
gem 'simplecov', :require => false
gem 'rspec-puppet-facts', :require => false
Copy link
Contributor

Choose a reason for hiding this comment

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

needs aligned

@tphoney
Copy link
Contributor

tphoney commented Jun 27, 2016

@DavidS +1 apart from the alignment

%w(lib manifests templates metadata.json).each do |file|
scp_to host, "#{proj_root}/#{file}", "#{host['distmoduledir']}/ntp"
end
copy_module_to(host, :source => proj_root, :module_name => 'ntp')
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@gregohardy
Copy link
Contributor

I'm happy. If no one objects. I will merge when travis has passed.

@bmjen bmjen merged commit 9d5ea3f into puppetlabs:master Jun 27, 2016
@DavidS DavidS deleted the cleanups branch June 27, 2016 15:30
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.

4 participants