-
Notifications
You must be signed in to change notification settings - Fork 290
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
(GH-884) Update acceptance testing #887
Conversation
d5784ed
to
626f937
Compare
.travis.yml
Outdated
@@ -24,6 +24,54 @@ matrix: | |||
env: PUPPET_GEM_VERSION="~> 4" | |||
- rvm: 2.4.1 | |||
env: PUPPET_GEM_VERSION="~> 5" | |||
- rvm: 2.4.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this version of rvm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overlaps version use in latest Puppet 5 release, need newer Rubies to work with beaker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use 2.4.3 so it is the same as the latest puppet release? That will make testing easier for people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but the version used in travis for acceptance tests isn't actually what runs inside the container so no real impact on puppet compatibility. The only ruby version restriction for the acceptance tests is being supported by beaker and the various beaker support libraries.
I think the default Puppet version used in the tests inside the various containers is puppet-agent 1.10.0 which is Puppet 4.x. Should we force latest Puppet or stick with default from beaker-puppet_install_helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further analysis seems the various helpers to install Puppet with beaker don't yet support Puppet 5, at least not obvious. puppetlabs/beaker-puppet_install_helper#33
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh. We need Puppet 5 as I'm looking to drop Puppet 4 in the next version of this module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see what's involved getting Puppet 5 used. May be we just can't use the helper methods and have to explicitly bootstrap puppet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got Puppet 5 as the version installed for acceptance tests. PUPPET_INSTALL_VERSION
sets the default, leaving undefined results in latest puppet-agent
present in puppet5
repo.
.travis.yml
Outdated
services: docker | ||
env: BEAKER_set="centos-6-docker" | ||
bundler_args: | ||
script: sudo service docker restart ; sleep 10 ; bundle exec rake beaker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we restart docker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly never thought about it, just something I have been doing since while ago when I started doing acceptance tests. May be unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please test and see if we need this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the service restarts and tests still work so was unnecessary.
@@ -1,6 +1,6 @@ | |||
source ENV['GEM_SOURCE'] || "https://rubygems.org" | |||
|
|||
if puppetversion = ENV['PUPPET_GEM_VERSION'] | |||
if puppetversion = ENV['PUPPET_GEM_VERSION'] || "~> 5.x" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should already choose the latest version. Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Gemfile errors out if PUPPET_GEM_VERSION
is not set because puppetversion
ends up being nil. Without this change PUPPET_GEM_VERSION
would have to be defined to the acceptance tests even though it has not bearing on the version of Puppet used by the tests.
$ bundle install
The latest bundler is 1.16.1, but you are currently running 1.15.4.
To update, run `gem install bundler`
[!] There was an error parsing `Gemfile`: undefined method `<' for nil:NilClass. Bundler cannot continue.
# from /Users/tdockendorf/git/sensu-puppet/Gemfile:37
# -------------------------------------------
# gem 'puppet-lint-version_comparison-check', :require => false
> if puppetversion < '5.0'
# gem 'semantic_puppet', :require => false
# -------------------------------------------
docker_cmd: | ||
- '/sbin/init' | ||
docker_image_commands: | ||
- 'yum install -y tar wget cronie git upstart' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are these dependencies determined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are things you may need to bootstrap test environment. I always do git incase git cloning modules for testing. For something like upstart
is needed to support docker_cmd
of /sbin/init
which is necessary to have functioning container for tests. Others may be unnecessary but always added as sometimes useful. cronie
likely not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further investigation, I believe cronie may have been added to support potentially adding cron jobs via Puppet in other modules and being able to test cron with serverspec: http://serverspec.org/resource_types.html#cron. Not 100% certain if serverspec uses crontab or looks in /var/spool/cron type situation.
platform: el-6-x86_64 | ||
hypervisor: docker | ||
image: amazonlinux:2017.03 | ||
docker_preserve_image: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allows container to be re-used between tests, useful for testing changes on local systems.
https://github.com/puppetlabs/beaker-docker/blob/master/docker.md#preserve-docker-image
These YAML may need to be updated to also allow BEAKER_provision=no
which has options likely needed with beaker 3.x to fully support re-using containers.
https://github.com/puppetlabs/beaker-docker/blob/master/docker.md#reuse-docker-image
I'll update all images as really handy when doing local development to be able to re-run tests. There is a bug though where executions after first will put the module in /etc/puppet/modules for some reason which is not used by AIO Puppet. Never got around to opening an issue with Puppetlabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - reusable docker images works as documented but annoying bug of module being installed to wrong place is still present:
centos-7 20:07:02$ echo /etc/puppet/modules
/etc/puppet/modules
centos-7 executed in 0.12 seconds
Using scp to transfer /home/treydock/git/sensu-puppet to /etc/puppet/modules/sensu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please report this bug to Puppet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue has changed. I'm pretty sure I was mislead by fact my initial local environment had beaker pinned to ~> 2.x
which created the container and then upgraded to latest beaker without re-creating container. Now I can't reuse containers at all which doesn't affect travis-ci, just means all tests have to run in fresh container.
@@ -0,0 +1,10 @@ | |||
HOSTS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a vagrant node if we have docker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not needed, would simplify the naming of docker containers. I have run into edge cases where some things don't work in docker containers, like modifying iptables, due to kernel modules not being able to load since kernel is of host and not container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are not testing it with TravisCI, this seems like cruft that could be removed. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done, all nodesets are docker now.
spec/acceptance/nodesets/default.yml
Outdated
platform: el-7-x86_64 | ||
box : puppetlabs/centos-7.0-64-nocm | ||
box_url : https://vagrantcloud.com/puppetlabs/boxes/centos-7.0-64-nocm | ||
box : centos/7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the default invoked? Why is it vagrant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe default is invoked if no BEAKER_set
environment variable is set. Left vagrant for compatibility but could just as easily make it docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a default really needed? If not, let's remove it, else let's make it docker.
- 'apt-get install -y -q net-tools wget' | ||
- 'locale-gen en_US.UTF-8' | ||
CONFIG: | ||
type: aio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are some types aio
and some are foss
? What is the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be foss
. I pulled the ubuntu files from one of my other puppet modules and the files were actually a contribution. I know the determination of what is installed is actually not in beaker and not from nodeset file, not sure if type
even used but it is here: https://github.com/puppetlabs/beaker/blob/master/acceptance/fixtures/module/spec/acceptance/nodesets/default.yml
Real place Puppet install type is defined: https://github.com/puppetlabs/beaker-puppet_install_helper#run_puppet_install_helper
Probably just need to make them all foss
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make them all foss
if that's what they should be.
@@ -1,7 +1,7 @@ | |||
HOSTS: | |||
ubuntu-server-1204-x64: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't support ubuntu 12. I don't think this should be here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove any platforms we do not support.
@@ -1,7 +1,7 @@ | |||
HOSTS: | |||
ubuntu-server-1404-x64: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have Ubuntu and Ubuntu Server? I don't get the distinction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the server
was left overs from previous vagrant boxes. I don't know of a difference, the containers are all just ubuntu
, no server distinction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove all the cruft we don't need.
@ghoneycutt I can squash commits once everything looks good if you'd prefer a single commit. |
.travis.yml
Outdated
@@ -24,6 +24,54 @@ matrix: | |||
env: PUPPET_GEM_VERSION="~> 4" | |||
- rvm: 2.4.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we should be using ruby 2.4.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated PR to use 2.4.3.
58fc522
to
19d775e
Compare
Add travis-ci tests for all supported versions of Linux that use docker nodesets Default PUPPET_GEM_VERSION to be Puppet 5 to avoid errors when not defined in environment
Remove docker nodeset docker suffix Make all nodesets type "foss" Get all nodesets capable of being rerun with BEAKER_destroy=no and BEAKER_provision=no combinations
19d775e
to
2fef933
Compare
2fef933
to
e7d1f47
Compare
Version set by environment variable PUPPET_INSTALL_VERSION
72bf8e2
to
69e6a13
Compare
end | ||
on host, puppet('module', 'install', 'puppetlabs-stdlib'), { :acceptable_exit_codes => [0,1] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does stdlib go away here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulled in from metadata.json
by install_module_dependencies_on(hosts)
at the top of file. Removed to avoid having to update some dependencies in two places.
Released in v2.53.0 |
Add travis-ci tests for all supported versions of Linux that use docker nodesets
Default PUPPET_GEM_VERSION to be Puppet 5 to avoid errors when not defined in environment
Pull Request Checklist
Description
Add automated acceptance tests.
Related Issue
Fixes #884
Motivation and Context
Improved module testing by adding automated system acceptance tests.
How Has This Been Tested?
Tested all supported Linux systems via travis-ci acceptance tests
General
Update
README.md
with any necessary configuration snippetsNew parameters are documented
New parameters have tests
Tests pass -
bundle exec rake validate lint spec