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 parameters for configuring package and service #31

Merged
merged 24 commits into from
Mar 18, 2017

Conversation

dhollinger
Copy link
Member

Added parameters that will allow for more granular control of the autofs package and service state.

closes #21

Updated the global Package resource ensure attribute to
look at the $autofs::package_ensure parameter. This is to
add customizability, as well as, prepare for the class
change to private.
Now that all the params are placed in the init.pp and the
package.pp and service.pp files look to the params in the init.pp,
the old tests were no longer passing. I removed the package_spec
and service_spec. Any tests in those files not already covered
in the autofs_spec have been added to the autofs_spec.
Removed the hasstatus and hasrestart parameters from init as
they are really unnecessary to the autofs service, which by
installing the package will already have a hasstatus and
hasrestart set to true.
Updated the osfamily and operatingsystem fact calls to use
the facts hash instead of direct calls.
The osfamily fact lookup has been replaced by the lookup for
facts['os']['family'].
The autofs_spec tests now use on_supported_os.each do from the
rspec-puppet-facts gem. This was required to prevent errors
related to not mocking osfamily or operatingsystem. It also
improves the scope for which the tests run as the validate against
all supported OS facts instead of just a manually entered subset
If the $package_ensure is set to 'absent', then it will not
include the service class.
@dhollinger dhollinger added this to the v3.0.0 milestone Mar 17, 2017
@dhollinger
Copy link
Member Author

Added commit to fix Rubocop failures

@vinzent
Copy link
Contributor

vinzent commented Mar 17, 2017

Remove service disabled test until we know why it fails on ubuntu1404

instead of removing and loosing knowledge about there is the possibility to add a skip tag:

https://relishapp.com/rspec/rspec-core/v/3-4/docs/pending-and-skipped-examples/skip-examples

https://github.com/voxpupuli/puppet-puppetserver/blob/master/spec/unit/puppet/provider/puppetserver_config/augeas_spec.rb#L9

@dhollinger
Copy link
Member Author

@vinzent That's a much better idea.

Service disable test fails on Ubuntu 14.04 (ONLY on ubuntu 14.04).

skipping the test in rspec until we can figure out why it is only
failing on Ubuntu 14.04.
Syncing my macbook with the work I did last night at home reverted
some of the rubocop fixes I made. This puts those fixes back in
Add and skip the service enable check until we can figure out
why it fails on Ubuntu 14.04. It doesn't appear to be a Puppet issue
as the command line tool shows that puppet sees it as disabled.

Looking at Serverspec and SpecInfra for the culprit
@dhollinger
Copy link
Member Author

dhollinger commented Mar 17, 2017

Ok - status update on service enable check:

It is currently skipped until a PR/fix is provided to the specinfra gem. The Puppet Upstart provider disables a service by creating a /etc/init/<service>.override file with content manual. This is what the puppet service provider does for Upstart when enable => false.

specinfra, however, tests Upstart for service enable by checking /etc/init/<service>.conf for content start instead. This causes the rspec tests to think that the service is still enabled when, in fact, it is not.

https://github.com/mizzy/specinfra/blob/master/lib/specinfra/command/debian/base/service.rb#L5

Thanks to @vinzent for the help!

@yastupin
Copy link

@dhollinger if Solaris support is definitely dropped, shouldn't we add a line on the README.md, moving it to unsupported and maybe explaining why?

@dhollinger
Copy link
Member Author

@yastupin That's still in flux and will be decided later. Right now, I think the conversation that I had with @alexjfisher and brief input from @rnelson0 - we should be able to maintain it as a "self-support" option. The unit tests have been built around checking for Solaris, our only pain point is that we don't have any beaker tests for Solaris.

Either way, this PR is dealing more with a couple of new features for managing the autofs package and service. This is just one of several issues slated for the 3.0.0 release

default: {
fail("${::operatingsystem} not supported.")
fail("${facts['operatingsystem']} not supported.")

Choose a reason for hiding this comment

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

@dhollinger maybe keep the empty Solaris block here then? otherwise the immediate fail() seems somewhat incompatible with self-support

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Nice catch


opsys.each do |os|
context 'main init tests' do
on_supported_os.select { |_, f| f[:os]['family'] != 'Solaris' }.each do |os, facts|
Copy link
Member

Choose a reason for hiding this comment

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

_, f
what kind of magic is this

let(:facts) do
facts.merge(concat_basedir: '/etc')
end
it { is_expected.to compile }
Copy link
Member

Choose a reason for hiding this comment

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

can we do compile.with_all_deps?

Copy link
Contributor

Choose a reason for hiding this comment

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

eep, sorry @bastelfreak, saw that it was approved and @dhollinger had asked for a merge earlier in the day; didn't see that these comments were so recent.

@wyardley wyardley merged commit 789beb9 into voxpupuli:master Mar 18, 2017
@wyardley
Copy link
Contributor

Also, the commits weren't squashed, if that's a problem @ mention me, and I can revert and either have @dhollinger squash them or just squash-merge.

@dhollinger dhollinger deleted the 21-pkg-svc-params branch March 24, 2017 17:28
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.

Add parameters for managing the package and service state
5 participants