Created attachment 296617 [details] [review]
patch file for sysadmins manifest

-1 here. changing 'true' to true is technically correct, but can cause chaos, since you are changing the type of the object from string to bool. If the original comparison was string based, it will break. Did you verify it?

(In reply to matanya from comment #2)
> -1 here. changing 'true' to true is technically correct, but can cause
> chaos, since you are changing the type of the object from string to bool. If
> the original comparison was string based, it will break. Did you verify it?

Thanks for the review. There are two instances of this in the tree, the one here and also ostree/manifests/init.pp.  The docs on the internet all indicate that managehome should be unquoted but they are quoted in these branches.

puppet --debug --verbose test.pp    returns no errors
puppet parser validate init.pp     returns no errors

I was unable to get a dryrun ("puppet agent --test --noop") to work though:
"err: Could not request certificate: The certificate retrieved from the master does not match the agent's private key."  The help text asks me to "puppet cert clean bastion.gnome.org" which I do not want to attempt.

Thoughts?

You will not be able to run puppet without the needed rights, as puppet checks the cert and the key before running, i don't know the exact setup on gnome, but probably running that with sudo (i you had rights) would work, in the case the cert and key match.

Created attachment 296627 [details] [review]
Modified patch with quotes as per matayna

This is not what i said :) I asked you to verify there is no string comparison that has managehome == 'true'. If you are sure there is no such case, changing 'true' to true is safe.

The sysadmin module is currently unused as we manage users through FreeIPA and not through Puppet as mentioned on #sysadmin yesterday. (10th Feb 2015)