search

dayglojesus / managedmac

Comprehensive Puppet module for OS X.
http://dayglojesus.github.io/managedmac/
Apache License 2.0
62 stars 21 forks source link

Desktop Picture class #66

Closed clburlison closed 9 years ago

clburlison commented 9 years ago

Allows us to set the desktop picture via a profile. Currently there is a bug with this profile such that setting the lock value to true or false has no affect. If the admin sets the desktop picture using this class the end user can not modify their background picture. It does have some use cases though.

I will just leave this here and see if you have any feedback.

dayglojesus commented 9 years ago

I had this feature on to-do list, so it is a welcome addition. Question: Does it work on 10.9 or just 10.10? Comment 1: I feel like the param names are a bit wordy. I may elect to refactor them. Comment 2: While I'm no expert at writing specs for Puppet, I have made it my goal to always include a very simple integration test for each class.

clburlison commented 9 years ago

I shamelessly didn't test the class in 10.10 but I did test a manually created profile, so on that front it should work. The class 100% does work for 10.9.

Comment 1: I lean towards having more descriptive variable names but would be happy to refactor if you have a preference.

Comment 2: I've only just started working on an implementation for puppet so by no means am I an expert either. I can try to get a test for this class created later this week.

Comment 3: I am also going to file a bug report on the locked key. I would like to leave it in the class for future support but hate that we can't allow users to override the value defined.

Since I haven't put this anywhere else...thank you SO much for your work on this. The detailed and well formatted instructions are extremely helpful to those new to puppet.

Clayton Burlison Sent from my iPhone

On Feb 9, 2015, at 11:22 AM, Brian Warsing notifications@github.com wrote:

I had this feature on to-do list, so it is a welcome addition. Question: Does it work on 10.9 or just 10.10? Comment 1: I feel like the param names are a bit wordy. I may elect to refactor them. Comment 2: While I'm no expert at writing specs for Puppet, I have made it my goal to always include a very simple integration test for each class.

— Reply to this email directly or view it on GitHub.

clburlison commented 9 years ago

Alright, I have looked into creating a spec for this class and it is evident I don't know what I am doing on that front. Hopefully you will can one for this class. Let me know if there is anything I can do to help.

dayglojesus commented 9 years ago

@clburlison are you sure that the locked key is an Integer and not a Boolean. Could this be contributing to the behaviour you describe of not being able to enforce the setting?

dayglojesus commented 9 years ago

Greg's example indicates the type is Boolean. I will change this.

https://github.com/gregneagle/profiles/blob/master/desktop_picture.mobileconfig#L20-L21

clburlison commented 9 years ago

That is a typo in the Parameters section. https://github.com/clburlison/managedmac/blob/3e3a49e0fd0711007f184e1f78e2a0d277ecb031/manifests/desktoppicture.pp#L23. The rest of the class has the boolean value as it should. The issue with this profile is with Apple and it could be the way I am thinking the value should work. At this moment in time the the locked key has no affect if it is enabled or disabled. When this profile is applied the picture will be the assigned picture until the profile is removed.

dayglojesus commented 9 years ago

Ah, I see that now. Thanks.

dayglojesus commented 9 years ago

Hey, man... Thanks a lot for this patch. Have a look at the mods. I did rename the class and params to align more with the PayloadType and key names in the Apple schema and supplied a spec. Cheers!

clburlison commented 9 years ago

Awesome job on this. I got a small bug from one of your commits (will submit separate issue) but the desktop class works and to my knowledge you cleaned up all the little bits. :+1: