The current state of PuppetAgain supports only CentOS since there are modules that contain hard stop for all non-centos clients. e.g. default: { fail("cannot install on $operatingsystem") } Callek and Kmoir can probably expand on this since I'm sure there are more things to change to achieve Darwin compliance in PuppetAgain.

My approach when a package doesn't apply is to add the operating system but leave the stanza blank. For instance, on Darwin vim and nano are installed by default so we don't have to specify them in puppet. Thus in the editors.pp I just added lass packages::editors { case $operatingsystem { CentOS: { package { "nano": ensure => latest; "vim-minimal": ensure => latest; } } Darwin: { } default: { fail("cannot install on $operatingsystem") } } }

to provide 10.8 support for sudoers module

Comment on attachment 635936 [details] [diff] [review] patch Review of attachment 635936 [details] [diff] [review]: ----------------------------------------------------------------- Pretty minor comments overall, one major issue but easily fixed ::: modules/sudoers/manifests/init.pp @@ +2,5 @@ > > + $group = $operatingsystem ? { > + Darwin => wheel, > + default => root > + } How about we do a sudoers::settings::group that sets this for us, where we can: include sudoers::settings then: $sudoers::settings::group where we need to elsewhere (rather than duplicating this switch everywhere)? @@ +22,5 @@ > + require => Package[sudo], > + mode => "440", > + owner => root, > + group => $group, > + source => "puppet:///modules/sudoers/$sourcefile"; For this, I would be happier with: "puppet:///modules/sudoers/sudoers.$operatingsystem" but you'll need to rename the current sudoers file, and change your sudoers file name. I think that will be more readable, and faster to identify when staring at |puppet:///modules/sudoers/sudoers" itself that its a linux-only file @@ +36,5 @@ > + } > + Darwin: { > + package { > + "sudo": > + ensure => installed; We're hitting: Fri Jun 22 13:25:53 -0700 2012 /Stage[main]/Sudoers/Package[sudo]/ensure (err): change from absent to present failed: Mac OS X PKG DMG's must specify a package source. according to my e-mail. ::: modules/sudoers/manifests/reboot.pp @@ +26,5 @@ > + mode => "440", > + owner => root, > + group => $group, > + ensure => file, > + content => "${::config::builder_username} ALL=NOPASSWD: /usr/bin/reboot\n"; If these two are identical (especially since you case the group earlier, or could use my settings suggestion) I would merge these to one section, or rip it out to no-OS checks

(In reply to Kim Moir [:kmoir] from comment #1) > My approach when a package doesn't apply is to add the operating system but > leave the stanza blank. FWIW I agree with this approach. > Darwin: { > } For this part in particular I would suggest a // installed by default So we can tell at a glance that it is there, and not UNIMPLEMENTED ;-)

I tried adding the settings module but when I included it in other modules, it didn't include correct value, it had the value of an uninitialized variable. For modules/sudoers/manifests/reboot.pp, they aren't the same, the CentOS one has require => Package[sudo], which isn't useful on the Mac because it requires a dmg source for a similar command and sudo is included in the base os.

Comment on attachment 635988 [details] [diff] [review] patch for sudoers suppport on 10.8 Review of attachment 635988 [details] [diff] [review]: ----------------------------------------------------------------- I know you didn't officially r? me on this posted patch, but figured I'd skim it anyway. Let me know if you disagree with what I suggest/said here (I don't want to stall with bikeshed comments, but I do want the shed to look pretty) ::: modules/sudoers/manifests/init.pp @@ +2,5 @@ > > + $sourcefile = $operatingsystem ? { > + Darwin => sudoersDarwin, > + default => sudoersCentOS > + } With my last comment I was meaning to basically get rid of the $sourcefile variable for this. That said we can also simplify this, with the wheel/root variable even if local to this file. @@ +9,5 @@ > + case $operatingsystem { > + CentOS: { > + package { > + "sudo": > + ensure => latest; if we add a before => File['/etc/sudoers', '/etc/sudoers.d'] .... @@ +14,5 @@ > + } > + file { > + "sudoers": > + path => "/etc/sudoers", > + require => Package[sudo], we can get rid of the require here... @@ +18,5 @@ > + require => Package[sudo], > + mode => "440", > + owner => root, > + group => root, > + source => "puppet:///modules/sudoers/$sourcefile"; ... use the magic $operatingsystem facter variable here, and mark group with the var.... @@ +29,5 @@ > + group => root, > + ensure => directory; > + } > + } > + Darwin: { and collapse this section to have all these file{} resources outside of the OS case. ::: modules/sudoers/manifests/reboot.pp @@ +6,5 @@ > + case $operatingsystem { > + Centos: { > + file { > + "/etc/sudoers.d/reboot": > + require => Package[sudo], and we can collapse this out because on linux we already have a dependency chain of: Package['sudo'] -> File['/etc/sudoers.d'] And in both linux and Mac, this creates a (auto) dependancy chain of: File['/etc/sudoers.d'] -> File['/etc/sudoers.d/reboot'] (puppet makes sure that if we define parent directory that it is started/created first before the file in it, and in this case we manage the directory and explicitly won't manage it until Package[sudo] is created.

much cleaner, added settings class. Thanks Callek for your suggestions.

notes: ntpd is installed by default on 10.8 image. changed to ntpd from ntpdate in atboot because ntpdate isn't a service on the machine I was testing with, not sure if this should be the case On Darwin, you can only run the ntpdate service, not nptd The set-time-server command is identical to the one used in the old puppet configs on Lion.

Today I was trying to update the users::builder module so it would work on 10.8. The error messages I kept getting was "err: /Stage[main]/Users::Builder/User[cltbld]: Could not evaluate: undefined method `string' for nil:NilClass" which suggests that a value wasn't being properly loaded from a template but both the username and password are fine when I run in debug mode. Not sure why this is happening, has anyone else seen this issue?

Comment on attachment 636169 [details] [diff] [review] updated patch for sudoers suppport on 10.8 Review of attachment 636169 [details] [diff] [review]: ----------------------------------------------------------------- The splinter review puts my comments in random order, but hopefully this is clear enough. ::: modules/sudoers/files/sudoersDarwin @@ +43,5 @@ > + %wheel ALL=(ALL) NOPASSWD: ALL > + > +# Samples > +# %users ALL=/sbin/mount /cdrom,/sbin/umount /cdrom > +# %users localhost=/sbin/shutdown -h now Does this need the same #includedir /etc/sudoers.d that appears in the CentOS version? Or does that not work on Darwin11? According to the manpage on my Snow Leopard machine, it does work.. ::: modules/sudoers/manifests/custom.pp @@ -3,4 @@ > > file { > "/etc/sudoers.d/$title": > - require => Package[sudo], require => Class['packages::sudo'] @@ +6,3 @@ > mode => "440", > owner => root, > group => root, This needs to be $sudoers::settings::group, too, right? ::: modules/sudoers/manifests/init.pp @@ +4,2 @@ > > + $group = $settings::group I think you want $sudoers::settings::groups here? @@ +10,5 @@ > + "sudo": > + ensure => latest; > + } > + } > + } So, this part should be in a new packages::sudo (which will be a no-op on Darwin). Then you can require it in other resources with Class['packages::sudo'], and that will work everywhere (Darwin and CentOS). @@ +18,4 @@ > mode => "440", > owner => root, > + group => $group, > + source => "puppet:///modules/sudoers/sudoers$operatingsystem"; I'd like a dot here, so this is sudoers.Darwin and sudoers.CentOS. Maybe it's my StrongAversionToJavaSpellings :) ::: modules/sudoers/manifests/reboot.pp @@ +4,3 @@ > include config > + > + $group = $settings::group Same here -- $sudoers::settings::group @@ +10,1 @@ > mode => "440", And this can require => Class['packages::sudo']. This is one of the best parts about abstracting things with classes, actually :)

Comment on attachment 636356 [details] [diff] [review] patch to add ntpd support for 10.8 Review of attachment 636356 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/ntp/manifests/atboot.pp @@ +4,4 @@ > case $operatingsystem { > CentOS: { > service { > + "ntpd": We definitely do *not* want to run ntpd on an atboot configuration -- just ntpdate on startup. So this should stay at its existing value (ntpdate) @@ +14,5 @@ > + "ntpdate": > + enable => true, > + hasstatus => false; > + } > + } This looks identical to that for CentOS, once the above is fixed. Maybe collapse the two into one case? ::: modules/ntp/manifests/daemon.pp @@ +26,5 @@ > + subscribe => File["/etc/ntp.conf"], > + enable => true, > + hasstatus => true, > + ensure => running; > + } It looks like this drops the ntpdate service on CentOS? That service sets the time on startup, so it should be kept around in addition to the ntpd service (which keeps the time correct). @@ +35,5 @@ > + enable => true, > + hasstatus => false; > + } > + exec { > + "set-time-server": command => "/usr/sbin/systemsetup -setnetworktimeserver ${ntpserver}"; This should be protected somehow from repeated execution. That can happen in one of two ways: either write the current value of $ntpserver to a file somewhere, and subscribe to that File resource (so that when the value changes, the exec gets re-run). You can see an example of this in the ccaceh module. The other option is to add another command to run systemstetup -getnetworktimeserver and compare the result. Probably the first is easier in this case. In either case, we want puppet to make it clear when it's *changing* this value, so the exec-unless-command trick isn't quite as silly as it sounds. @@ +51,5 @@ > enable => false, > hasstatus => false; > } > + } > + Minor, but it'd be nice to get rid of these whitespace changes. ::: modules/packages/manifests/ntp.pp @@ +6,5 @@ > ensure => latest; > } > } > + Darwin: { > + #ntpd is installed with base install image I like this!

Updated ntpd patch. Note that there is a modification to the users module in this patch. There are many more modifications to the users module that I have in another patch that is forthcoming.

Comment on attachment 637226 [details] [diff] [review] updated patch for sudoers Review of attachment 637226 [details] [diff] [review]: ----------------------------------------------------------------- Looks great - sorry for the delay!

Comment on attachment 637490 [details] [diff] [review] patch to add ntpd support for 10.8 Review of attachment 637490 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! I'm not sure ~cltbld is the best place to store a semaphore file like that, but I can't think of anywhere better, so let's do it :)

Comment on attachment 637226 [details] [diff] [review] updated patch for sudoers Review of attachment 637226 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/packages/manifests/sudo.pp @@ +7,5 @@ > + } > + } > + Darwin : { > + # installed by default > + } nit: indent level (tabs and spaces used here, so might look fine locally, please choose one ;-) ) ::: modules/sudoers/files/sudoers @@ -1,1 @@ > -## Sudoers allows particular users to run various commands as nit ensure this file (modules/sudoers/files/sudoers) is hg renamed to the .CentOS version, (git made patches don't track that in an hg-recognizing way, easily -- so this patch shows it as (removed).) ::: modules/sudoers/manifests/init.pp @@ +4,5 @@ > > + file { > + "sudoers" : > + require => Class['packages::sudo'], > + path => "/etc/sudoers", nit: indent again.

Ah, I didn't look at whitespace. The only thing I despise more than tabs is religious wars over them, so .. please use spaces, but if you don't, I won't say anything :) (incidentally, github did wonders for the tab-elimination crusade by rending them as two spaces in their web interface, making just about every source file containing tabs look awful)

This patch adds support for to the users module for 10.8. Since currently this platform will be used for testing only, not building I just added the packages that are installed on 10.7 for testing purposes, and the associated directories. The part copying the plist files are currently commented out until I have access to a final 10.8 machine. Some of the binaries included in the users/files directory aren't included in the patch, not sure if they should reside there or in the appropriate /data directory on the server.

Comment on attachment 639450 [details] [diff] [review] patch to add users support for 10.8 Review of attachment 639450 [details] [diff] [review]: ----------------------------------------------------------------- A bunch of the stuff in user::builder is not particularly user-related - especially the stuff about /usr/local. The directories there might do well under the 'dirs' module. I'm not sure what blueutil is (since its source isn't in this patch, along with a few other puppet:///modules/user/* things). Can we move that to another module(s)? That module can certainly require Class['user::builder'] and reference the builder_username for things that need to run interactively or as a logged-in user. The turn-on-ssh could go in an sshd module (where we might add sshd_config later, or maybe a host-global known_hosts). The indexer disablement could go into the module that removes unnecessary services. The ~/bin/* files are fine to leave in the user::builder module, since they relate to its homedir, but I'm curious what they are and what they're used for. Should they exist on all machines, or just Darwin? We shouldn't be checking in commented-out code - either uncomment, or remove. I'd rather not have commands that run on every puppet invocation, as those will appear in the dashboard as changes made on every run. I think the 'tidy' resource would work better for removing saved state, rather than exec. Ideally (not in this bug!) cleanup like that would happen outside puppet, maybe after the puppet run. Whew! I had a lot to say here, but this is pretty close. The r- is mostly because it looks like there are syntax errors, there's no user { .. }, and there's all sorts of extra stuff in users::builder. It shouldn't be a long trip to get that cleaned up. And we're building the foundations for something great here, so it's worth getting right :) ::: modules/packages/manifests/mozilla/py27_mercurial.pp @@ +13,5 @@ > + "mercurial-1.3.1.dmg" : > + provider => pkgdmg, > + ensure => installed, > + source => "puppet:///repos/DMGs/mercurial-1.3.1.dmg" ; > + } I assume this DMG is from the existing puppet. Holy cow, 1.3.1 is old! Do you have any idea where this DMG came from, or how to recreate it with a newer version? Maybe that's something to do in a followup bug, if 1.3.1 is adequate for now. ::: modules/packages/manifests/mozilla/python27.pp @@ +6,5 @@ > ensure => latest; > } > } > + Darwin: { > + # installed by default It is installed, but it's not at the same /tools/ path as the RPM. Maybe this should just install the necessary symlinks? ::: modules/users/manifests/builder.pp @@ +20,5 @@ > + $group = $users::settings::group > + > + case $operatingsystem { > + CentOS : { > + user { Why no user { .. } for OS X? We'll ant to at least manage password there. @@ +73,5 @@ > + > + > + case $operatingsystem { > + Darwin : { > + require packages::screenresolution file { This doesn't look like correct syntax? @@ +88,5 @@ > + owner => "root", > + group => "$group", > + mode => 755 ; > + > + "/usr/local/bin/check-for-package.sh" : This gets installed, but never used. Is it necessary?

Per IRC, there's some trouble with the user resource type on Darwin: /Stage[main]/Users::Builder/User[cltbld]: Could not evaluate: undefined method `string' for nil:NilClass http://www.mail-archive.com/puppet-bugs@googlegroups.com/msg37324.html suggests this has to do with the plist format, but we can't see that bug, sadly, presumably because of the NDA. Kim, it looks like the upshot is that we will need to stick with the exec-and-dscl solution for now, but please put a bug on file (you can assign to me if you'd like) to do it the right way once that works!

As Dustin mentioned, there's currently a bug with adding a user via users on 10.8 so I'm using dscl and exec. The unfortunate thing is that I can't find a way to add a hashed password this way. I changed the mercurial dmg installed to a newer one, the old one was installed on the other mac test machines. I believe the /usr/local/bin/check-for-package.sh is used during the dmg installation to create a file in /var/db/.puppet_*package* name so this can be checked later to see if the package is installed, this was in the old lion configs for puppet. Lines like require packages::screenresolution file aren't syntax errors, that's just the default formatting in my puppet editor (Geppeto) which I'll fix. This patch is still a work in progress :-)

I *think* check-for-package.sh was used with install-dmg.sh, which together provided the functionality now supported by the pkgdmg provider. Try removing it and see if things break? Maybe I'm wrong! Looking good!

I removed a some items that will need to be added back later. For instance screenresolution doesn't work on my test machine now, I think it's a configuration issue because it needs a dongle perhaps? The

Comment on attachment 642069 [details] [diff] [review] patch for 10.8 users support Review of attachment 642069 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good. Rather than try to re-add the things you removed, it might be best to take another crack at this set of stuff, and get it committed? There are a *lot* of moving pieces here, and knowing that some are nailed down will make the remainder seem less daunting. ::: modules/dirs/manifests/builds/slave.pp @@ +2,4 @@ > include dirs::builds > > file { > + ["/builds", "/builds/logs", "/builds/slave"] : File['/builds'] will conflict with the same resource in dirs::builds. And /builds/logs should be in the eponymous dirs::builds::logs. The dirs::* classes should exactly match their filenames. ::: modules/dirs/manifests/test.pp @@ +12,5 @@ > + mode => 755 ; > + } > + } > + } > +} Same here -- none of these are /test, which is what dirs::test should create. Why are these directories test-related, anyway? And this should use the $::settings::root_group variable, too. ::: modules/disableservices/manifests/common.pp @@ +19,5 @@ > ensure => stopped, > } > + exec { > + "disable-indexing" : > + command => "/usr/bin/mdutil -a -i off" ; This looks like it will run on every invocation of puppet. Is it just missing a "refreshonly => true"? @@ +28,5 @@ > + } > + file { > + "$users::settings::home_dir/.puppet-indexing" : > + content => "indexing-disabled", > + notify => Exec["disable-indexing, remove-index"] ; I think you'll need some additional quotes in that Exec: Exec['disable-indexing', 'remove-index'] Also, it's probably easier to put sentinel files like this in a location that will be the same on every host. Probably the easiest is to hard-code the dir to /var/lib/puppet. ::: modules/users/manifests/builder.pp @@ +74,2 @@ > source => "puppet:///modules/users/gitconfig"; > + It looks like this addition got removed accidentally when you were merging? @@ +93,5 @@ > } > + case $operatingsystem { > + Darwin : { > + file { > + "/usr/local/bin/check-for-package.sh" : Did you determine that this file is required (from comment 22 and comment 23)? @@ +102,2 @@ > > + "/var/log/screensaver" : What's this for? @@ +117,5 @@ > + mode => "4755", > + require => File["$home_dir/bin"], > + source => "puppet:///modules/users/chown_root" ; > + > + "$home_dir/bin/chown_revert" : And I'm still confused about the purpose of these chown_* scripts - how and where are they used? @@ +125,5 @@ > + require => File["$home_dir/bin"], > + source => "puppet:///modules/users/chown_revert" ; > + } > + exec { > + "disable-updater" : This should probably be in disableservices, too, right? It's not specifically cltbld-related. @@ +132,5 @@ > + "/usr/sbin/softwareupdate --schedule off | egrep 'off'" ; > + } > + #Probably too late at this point, but lets get rid of them for the next reboot > + tidy { > + "/Users/cltbld/Library/Saved\ Application\ State/*.savedState" : This has 'cltbld' hard-coded. ::: modules/users/manifests/settings.pp @@ +6,5 @@ > + default => '/home' > + } > + $home_dir = "$home_base/$config::builder_username" > + > + #files are owned by staff group on macosx specifically *users'* files.. this confused me for a bit :) @@ +11,5 @@ > + $group = $operatingsystem ? { > + Darwin => 'staff', > + default => $config::builder_username > + } > + $uid = $operatingsystem ? { This could use some comments, too. I think we talked in IRC about where it's required (I can't find it in the bug anyway).

Comment on attachment 643845 [details] [diff] [review] patch to use correct root dir on Darwin Sorry hit return too fast. The roothomedir is used in forthcoming patches too.

Comment on attachment 643845 [details] [diff] [review] patch to use correct root dir on Darwin Heh, I just added that in one of the patches I'm working on. I added mine to shared ($shared::root_home). But your idea makes more sense, since we have a lot of these user-specific variables now: $users::builder::username $users::builder::group $users::builder::home $users::root::username (for symmetry) $users::root::group $users::root::home So, please rename this to $home in the class (rather than $roothome), and commit. I'll rebase my stuff on top, and make sure the rest of those exist, and add docs.

Comment on attachment 643890 [details] [diff] [review] misc Darwin patches from my workspace I commented on the first hunk midway through comment 25, and those comments still apply. The second hunk looks fine.

including the suggestions from comment #25

Comment on attachment 645116 [details] [diff] [review] patch to disable the additional services on Mac Let's use a more stable location for semaphore files. In this case, "$vardir/.puppet-indexing" would do the trick. Then there's no need to use users::settings, which will conflict with bug 776641. r+ with that fix.

Given that Callek is working on bug 774157 with support for ssh support on the linux-foopy, I thought I'd ask for your feedback. Your bug is for client support and thus in a ssh class, this one is for sshd support on the Mac.

Comment on attachment 645270 [details] [diff] [review] patch to enable sshd on Darwin Checking for the listening socket might be easier: "netstat -na | grep -q 'tcp4.*\*.22.*LISTEN'" Also, why is the sshd::settings manifest enabling the service? Most of the other modules use xxx::settings to contain settings for the module. The service enablement should be in sshd::service or, in a simple module like this, just in sshd (i.e., init.pp).

Comment on attachment 645866 [details] [diff] [review] exclude yum repos from being setup up on Darwin The comment for the Darwin section should really just indicate there's nothing to set up on Darwin. The class itself isn't yum-specific (and this patch makes it behave as such!)

so this works on Darwin

Comment on attachment 646362 [details] [diff] [review] disable updates on mac was missing Hm, we should probably use a semaphore for that, but lgtm for now. I'll make a note in my "minor stuff" list.

Comment on attachment 645270 [details] [diff] [review] patch to enable sshd on Darwin I'll pre-emptively r+ this. Sorry, Callek :)

Comment on attachment 645270 [details] [diff] [review] patch to enable sshd on Darwin Whoops, Callek reminded me that I suggested a simpler 'onlyif' above. Also, please add the following for Darwin: # Delete the com.apple.access_ssh group. If present, this group limits # SSH logins to those in the group, but without it, any user can log in. group: { 'com.apple.access_ssh': ensure => absent; }

I tested it today and it worked fine. wget was missing when I was running test builds on my test master.

Comment on attachment 646715 [details] [diff] [review] patch to create wget dmg and install on Darwin Review of attachment 646715 [details] [diff] [review]: ----------------------------------------------------------------- So fortunately neither of these changes require re-building the DMG. r+ with these changes made, or I'm happy to look at the re-drafted patch. ::: modules/packages/manifests/mozilla/wget-dmg.sh @@ +10,5 @@ > +# variables to parallel the spec file > +realname=wget > +version=1.12 > +release=1 > +#_prefix=/tools/$realname This is a little confusing, since the package does not install in /tools/$realname, but directly in /usr/local. Incidentally, you can see the contents of a package with e.g., lsbom /Volumes/mozilla-wget-1.12-1/wget-1.12-1.pkg/Contents/Archive.bom ::: modules/packages/manifests/mozilla/wget.pp @@ +1,1 @@ > +class packages::mozilla::wget { And, I went back and forth about this, but I think this should just be packages::wget, since this isn't a custom package (just a hand-built version on OS X).

I updated the docs to indicate this https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain/HowTo/Hack_on_PuppetAgain#DMGs

Redraft of attachment 645270 [details] [diff] [review], in hopes it can help me rescue the host I lost access to in bug 776641.

I meant unless, not onlyif

Updated patch. Dustin, thanks for the command to look at the contents of a dmg, I didn't know that. Also, thanks for refactoring my sshd patch. I got up early to do that today and then saw that it was already done :-)

fixing typo in previous one

Comment on attachment 646796 [details] [diff] [review] bug764948-sshd.patch Review of attachment 646796 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok to me, forwarding r? to kim for the mac specifics I'm unsure about

Comment on attachment 647591 [details] [diff] [review] jetpack tests need hg installed, thus need hg on slaves And a nice illustration of the benefits of the toplevel pattern!

Comment on attachment 646796 [details] [diff] [review] bug764948-sshd.patch kmoir, ping?

Comment on attachment 646796 [details] [diff] [review] bug764948-sshd.patch Looks good to me.

Comment on attachment 646796 [details] [diff] [review] bug764948-sshd.patch We were seeing errors like this Thu Aug 02 15:08:48 -0700 2012 Puppet (err): Could not retrieve catalog from remote server: Error 400 on SERVER: Could not find class sshd::service for bld-centos6-hp-012.build.scl1.mozilla.com at /etc/puppet/production/modules/sshd/manifests/init.pp:2 on node bld-centos6-hp-012.build.scl1.mozilla.com Thu Aug 02 15:08:48 -0700 2012 Puppet (err): Could not retrieve catalog; skipping run so I reverted the patch

Hm, looks like the real error was err: Could not retrieve catalog from remote server: Error 400 on SERVER: Syntax error at ':'; expected '}' at /etc/puppet/environments/dmitchell/env/modules/sshd/manifests/service.pp:20 on node relabs08.build.mtv1.mozilla.com I'm not sure why the subsequent errors were different. I'm also not sure how I managed to test (and get reviewed, twice!) code with a typo in it like that. This patch is tested against relabs08, which is set up as a mock builder like the HP's, as well as an r5 mini. Minor detail: I'm going to mark attachment 646796 [details] [diff] [review] as checked-in+ since the revert wasn't a full revert (which reverses the entire changeset), just a disable.

Comment on attachment 648545 [details] [diff] [review] bug776641.patch I swear I'm not trying to trick you with these patches, but .. this one was backward! Anyway, I've landed it the right way 'round.

I'd like to test running buildbot on the mac slaves (see https://bugzil.la/759466#c11), this is patch enables the new talos slaves in puppet.

Comment on attachment 650559 [details] [diff] [review] patch to enable mac talos machines in puppet I think that should be "talos-mtnlion-r5-\d+.test"..., right? r+ with that change.

My editor was dirty in Eclipse (not saved) when I exported the original patch and thus the typo. I had the correct patch on the server and tested it and worked fine.

I don't think we need to explicitly disable the screensaver because the machines are all set to never sleep. I tested this by enabling the screen saver on one of the test slaves and it never started. This patch will also stop the errors in the puppet mail.

Comment on attachment 650589 [details] [diff] [review] patch to remove bogus support to disable screen saver OK, we'll certainly see test failures if this turns out to be a problem :)

Kim, even "trivial" changes need to be reviewed, unless they're an emergency break-fix! 1.1 --- a/modules/sudoers/manifests/reboot.pp 1.2 +++ b/modules/sudoers/manifests/reboot.pp 1.3 @@ -5,11 +5,11 @@ class sudoers::reboot { 1.4 include users::builder 1.5 1.6 file { 1.7 "/etc/sudoers.d/reboot" : 1.8 mode => $sudoers::settings::mode, 1.9 owner => $sudoers::settings::owner, 1.10 group => $sudoers::settings::group, 1.11 ensure => file, 1.12 - content => "${users::builder::username} ALL=NOPASSWD: /usr/bin/reboot\n" ; 1.13 + content => "${users::builder::username} ALL=NOPASSWD: /sbin/reboot\n" ; 1.14 } 1.15 } that's not a typo: [cltbld@relabs08 ~]$ which reboot /usr/bin/reboot So, you've broken reboots on the HP's.

The path for reboot was wrong for Darwin. Last night I fixed this not realizing that this also applied to Centos. I didn't get code review because there wasn't anyone in channel and I really wanted to get the 10.8 stuff into production. Lesson learned: never release anything without code review. Ever. Especially when you're tired and it's late. I added the include root::users in the sudoers:settings because it didn't work on my Centos test slave without this but perhaps this may not be needed.

Comment on attachment 652074 [details] [diff] [review] patch to point to correct path for reboot on Darwin and CentOS Review of attachment 652074 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. The users::root include is definitely required; I'm surprised (well, sad at puppet's nondeterminism) it worked at all without!

What's left here?

Just bug 779678 - fix vnc so linux clients can connect and bug 781240 - set desktop background to a solid colour

I think I'll close this. There are a few small bugs open for minor issues but the tests have been running fine for months now with the the PuppetAgain changes to support 10.8.