-
Notifications
You must be signed in to change notification settings - Fork 568
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 "basic" support for OpenBSD. #304
Conversation
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
$repo = true | ||
$homedir = '/var/lib/jenkins' | ||
$jenkinsdir = $homedir | ||
$default_plugin_dir = "${jenkinsdir}/plugins" |
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.
Might be worth lining up the indenting even though the linter won't complain about it.
I think I addressed all of your comments, Travis is still green, and it still seems to work for me ;) |
rebased for your convenience |
service { 'jenkins': | ||
ensure => $jenkins::service_ensure, | ||
enable => $jenkins::service_enable, | ||
flags => $jenkins::service_flags, |
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 this need to be it's own case? If flags is empty, it'll be fine, iirc.
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 had run into trouble with a different module I made aware of those flags. Short after merge upstream, someone who ran an older version of PE complained. That version he ran, I don't remember, wasn't aware of the flags parameterr to the service, and bailed out. Therefore the two cases here, If you don't care, I can have it without the case statement ;)
@buzzdeee There's upcoming OpenBSD support in Beaker (voxpupuli/beaker#812) so there might be the option to add a beaker spec for this when it gets merged! 👍 |
@@ -41,12 +41,17 @@ | |||
# service_ensure = 'running' (default) | |||
# Status of the jenkins service. running, stopped | |||
# | |||
# service_flags = undef (default) | |||
# Service parameters for the jenkins service on OpenBSD, string, see daemon_flags | |||
# in /etc/rc.d/jenkins for an example. |
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 there supposed to be a template for /etc/rc.d/jenkins
?
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.
nope, OpenBSD jenkins package comes with a well working rc script, see: http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/jenkins/pkg/jenkins.rc?rev=1.6&content-type=text/plain
@buzzdeee Got a working OpenBSD Beaker test 💃 : require 'spec_helper_acceptance'
describe 'jenkins class on OpenBSD', :if => fact('osfamily') == 'OpenBSD' do
before(:all) do
shell('rm -rf /etc/puppet/modules/java')
shell('pkg_add -I git')
shell('git clone https://github.com/puppetlabs/puppetlabs-java /etc/puppet/modules/java')
pkg_conf_pp = <<-EOS
$source = "http://ftp3.usa.openbsd.org/pub/OpenBSD/${::kernelmajversion}/packages/${::architecture}/"
file { '/etc/pkg.conf':
ensure => file,
owner => 'root',
group => 'wheel',
mode => '0644',
content => "# OpenBSD pkg.conf\ninstallpath=${source}\n",
}
EOS
apply_manifest(pkg_conf_pp, :catch_failures => true)
end
context 'default parameters' do
it 'should work with no errors' do
pp = <<-EOS
class {'jenkins':}
EOS
# Run it twice and test for idempotency
apply_manifest(pp, :catch_failures => true)
apply_manifest(pp, :catch_changes => true)
end
describe port(8000) do
it {
sleep(10) # Jenkins takes a while to start up
should be_listening
}
end
describe service('jenkins') do
it { should be_running }
end
end
end |
@petems, for the $source in the beaker test you might want to use: so ::hardwareisa instead of ::architecture, There are architectures/platforms, that differ, i.e. macppc/powerpc, and a few others. Do you want me to integrate that? Haven't used beaker before, so it would not incredibly easy to test for me ;) cheers, |
@buzzdeee I can pull it in as a separate PR 👍 |
See #342 |
that's fine with me, thanks! I kept the large number of changes for the time being, for the case you want to see better what I did. I can squash/rebase whenever necessary or wanted. |
$jplugin_dir = $plugin_dir ? { | ||
'' => $::jenkins::params::plugin_dir, | ||
default => $plugin_dir, | ||
} |
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 had to do above dance with the parameters,
I just tried to pull in
and also the others, in the parameter list of the defined type, but then all the time, the tests failed. It seemed that Puppet didn't pulled that info in at that time.
I had to set the default values for the parameters to '', because the selector didn't wanted to work when they were set to undef.
Alternatively, I could set these variables default to undef and do check like:
if $plugin_dir {
$jplugin_dir = $plugin_dir
} else {
}
if that is preferred, or let me know if there is a better way to work around that.
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.
A :pre_condition
block that pulls in jenkins/jenkins::params
would likely be needed for those tests to work. However, that would introduce evaluation order dependent behavior for users. My opinion is that introducing parse order requirements at this point would be a major API break.
Unlike the inherits
hack that works for classes, there's no way [that I'm aware of] internal to a defined type to require that a class be evaluated before it in the manifest. It can be done external to the define by placing constraints using resource collectors (which have side effects on virtual resources) or if you know the name of the define. I used the later approach in my selenium module. Eg., https://github.com/jhoblitt/puppet-selenium/blob/master/manifests/hub.pp#L14-L23 Which isn't exposed to the user as its wrapped up in a class. I think the if/else approach you've described or a custom function is the most robust approach.
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 figured that I can do something like:
define jenkins::plugin (
foo,
) {
Class['jenkins::params'] ->
Jenkins::Plugin[$name]
}
to ensure that the jenkins::params are evaluated before the define.
See PR #356 , first start of to configure the user/group in params.pp
@buzzdeee I'd like to suggest that you try to split all of the non-specific to openbsd values out of this branch and create a new PR for it. I think there's some valuable house keeping in this PR, in particular having a canonical value for the jenkins user/group, that should be fairly straight forward to merge. That should ultimately make it easier to keep this PR rebased on the current master. |
I'll see what I can do to split off the PR into multiple, just need to find the time, hopefully on the weekend, or next week. |
@buzzdeee I think if you rebase upon the current master this will be a much smaller PR now. |
yay, will hopefully find time this week cheers, |
Any update for this Issue ? I'm interesting in OpenBSD and FreeBSD support |
Jenkins installed via the jenkins package is a bit different on OpenBSD than on other Linux that were supported so far. Major differences: * jenkins system user/group is called _jenkins * jenkins users home directory is /var/jenkins, but the state and everything related to Jenkins is stored under /var/jenkins/.jenkins, i.e. /var/jenkins/.jenksin/plugins Therefore following changes: lib/puppet/jenkins.rb * figure out the jenkins home_dir based on the _jenkins user * point the plugins_dir to $home_dir/.jenkins/plugins lib/puppet/jenkins/facts.rb * confine the jenkins_plugins fact to OpenBSD too manifests/init.pp * add a service_flags parameter, to hand over flags for the jenkins service * while there, remove some redundant documentation of user/group parameters manifests/params.pp * make $user, $group, and $localstatedir dependent on ::osfamily * add a $jarbin variable, pointing to the Java jar binary, for all but OpenBSD, use the short name: 'jar', for OpenBSD, default to the full path where it can be found, overridable via parameter to init.pp * similarily to the $jarbin, add a $javabin variable and use it manifests/repo.pp * add an empty case for OpenBSD, no special handling required jenkins package is in default repos available manifests/service.pp * add a special case for OpenBSD to manage the service. This is necessary, because I've seen with other puppet modules, supporting older puppet versions, that the service_flags parameter might not be understood when it is given. manifests/cli/config.pp * do not hardcode the jenkins user and group * use the $::jenkins::jarbin variable, in order to find the Java jar binary manifests/cli.pp * make use of the $javabin variable instead of hardcoding the java binary manifests/cli_helper.pp * make use of the $javabin variable instead of hardcoding the java binary specs/* * rearrange/add some specs to handle tests for OpenBSD Add javabin parameter, and use it where java binary is used
of the cli tools that take info from the config_hash to find the jenkins port Fixup localstatedir for OpenBSD, point to .jenkins subdir rubocop pleasings
Hi people. This PR is pretty huge and got quite some feedback. But since it got stalled, I'm going to close it. If anybody is still interested in it, please reopen/resubmit it and rebase. |
Support only "basic", since a few things are untested, and known to not work (yet).
What works and is tested:
What is definately not working:
puppetlabs/java before that, to add a fact pointing to the right
directory, or add another variable to params.pp ;)
Otherwise untested:
Some changes had to be done to the params.pp. Some information that
were hard-coded before, are now OS dependent set there. For all the
other OS, kept as it was there already, or somewhere else hard-coded.
On OpenBSD, and other *BSD, there is no group 'root', therefore, whereever
access permissions for a 'root' group were made, changed it to the gid of '0'
The jenkins system user and group are '_jenkins', therefore added these
as variables. Jenkins users home directory is /var/jenkins, instead of the
otherwise hardcoded /var/lib/jenkins, and further, jenkins is installed
in /var/lib/jenkins/.jenkins.
Because of above facts, most changes to manifests/plugins.pp, because the
plugin_parent_dir assumption, that this is the same as the jenkins users
home directory is not true on OpenBSD. Also, wget is available as package,
but installed in /usr/local/bin. Therefore added this to the path for the command.
Further, made repo management OS dependent, and for OpenBSD, set the value
to false.
Services on OpenBSD get configured via flags to the OpenBSD service provider.
Since I had seen problems with older Puppet versions, manifests/services got
a little bit enhanced, to only make use of that flag, when on OpenBSD.
Last but not least, a bunch of specs added here and there, to reflect OpenBSD.
Tested on OpenBSD amd64 with Puppet 3.7.4, jenkins-1.605 and jenkins-1.596.1, that is all available as packages.
haven't added OpenBSD to metadata.json, due to above mentioned bits that are not (yet ;) supported.
Hope that all that makes sense, and can be merged. Let me know if there is something that needs clarification, or changes or whatnot, and I'll be happy to work on it.
cheers.