Refactor #5079

MCX content provider

Added by Sandor Szücs over 2 years ago. Updated over 2 years ago.

Status:ClosedStart date:10/22/2010
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:OSX
Target version:-
Affected Puppet version: Branch:
Keywords:

Description

I refactored the MCX content provider to enhance code readability. This is the first step to understand the current implementation.

I would like to add a best practice dslocal solution pointed out by the authors of “Enterprise Mac Managed Preferences”. They wrote that you should not use /Local/Default as DB, because it is also used as local cache. They further said that you should add an own DB to circumvent ping-pong overwrites between central managed MCX and dslocal managed MCX.

The patch includes a small change within the spec file, because the error handling should be more conform to other puppet providers. Instead to raise MCXContentProviderException I choose to raise Puppet::Error, because macauthorization provider does the same. I hope that is the right Error handling.

mcxcontent_provider_refactored.diff Magnifier (11 KB) Sandor Szücs, 10/22/2010 10:37 am

History

#1 Updated by Nigel Kersten over 2 years ago

  • Status changed from Unreviewed to Investigating
  • Assignee set to Nigel Kersten

That’s not quite true Sandor. I actually tech-edited that book with Greg and Edward, so I’m reasonably familiar with the contents :)

The suggestion in that book is that you shouldn’t use /Computers/localhost because it conflicts with caching. The problem only exists with Computer objects named ‘localhost’. The problems aren’t that severe, but they can confuse people, thus the suggestion in that book.

I agree we should support DSLocal nodes other than the default, because people are starting to do more experimentation with secondary DSLocal nodes, but there is nothing wrong with doing all your management in the default local domain.

Yes, you’ll have trouble managing the entire MCX dump for a given object if that object is also managed via remote MCX, but that problem only exists for Mobile Accounts, and is not an argument for not doing any MCX management, but that we should be shipping a simpler provider that allows you to set individual keys, rather than the entire MCX blob.

I wrote the macauthorization provider, and I’m not entirely convinced that my error approach in it was better than Jeff’s in the MCX one, so I wouldn’t take it as gospel.

We’ve seen a couple of useful patches from you, is there any chance we could get you to follow the development lifecycle and mail this to the dev-list for discussion?

http://projects.puppetlabs.com/projects/puppet/wiki/Development_Development_Lifecycle

I’m more than happy to give you a hand with this, as I have a strong personal interest in improving all our Mac providers, and would love to see more active involvement from community members.

#2 Updated by Sandor Szücs over 2 years ago

Nigel Kersten wrote:

That’s not quite true Sandor. I actually tech-edited that book with Greg and Edward, so I’m reasonably familiar with the contents :)

The suggestion in that book is that you shouldn’t use /Computers/localhost because it conflicts with caching. The problem only exists with Computer objects named ‘localhost’. The problems aren’t that severe, but they can confuse people, thus the suggestion in that book.

Thanks that you clarified this.

I agree we should support DSLocal nodes other than the default, because people are starting to do more experimentation with secondary DSLocal nodes, but there is nothing wrong with doing all your management in the default local domain.

Yes, you’ll have trouble managing the entire MCX dump for a given object if that object is also managed via remote MCX, but that problem only exists for Mobile Accounts, and is not an argument for not doing any MCX management, but that we should be shipping a simpler provider that allows you to set individual keys, rather than the entire MCX blob.

Ok. I should mention that I care about Mobile Accounts and maybe PHDs in the future. Also I am interested in mixing MCX settings in dslocal with remote MCX via LDAP (AD/OD) entries.

I wrote the macauthorization provider, and I’m not entirely convinced that my error approach in it was better than Jeff’s in the MCX one, so I wouldn’t take it as gospel.

Maybe error handling can be improved even more, but that should decide that have deeper knowledge in the source base than me. For me it is more obvious to use something like Puppet::Error then to define an own for this provider. Also counting with grep and wc showed 90 uses of Puppet::Error and 44 other (Puppet::DevError, ArgumentError and raise with default which is using RuntimeError).

We’ve seen a couple of useful patches from you, is there any chance we could get you to follow the development lifecycle and mail this to the dev-list for discussion?

Thanks I hoped there are useful. Last week I sent the patch to the list via rake task, so you can comment inline.

All the best, Sandor

#3 Updated by Matt Robinson over 2 years ago

  • Status changed from Investigating to Rejected

After some discussion on the mailing list with Sandor we’re rejecting this refactor since it doesn’t add any functionality. Sandor will include this refactor in a future patch if he finds something he’d like to add or change that builds on the refactor.

#4 Updated by Matt Robinson over 2 years ago

  • Status changed from Rejected to Code Insufficient
  • Assignee changed from Nigel Kersten to Sandor Szücs

After reviewing the patch further, it looks like the changes were good, it was just really hard to tell from the diff that they were good. I’ve responded on the mailing list with an example of how the patch could be presented so that it’s easier to read the diff and tell it was a refactor, and to separate the refactor changes from other changes. I didn’t make the change to the error type or pulling the “Default” out into a constant, so Sandor, if you’d like to refactor your commits to make it clearer, or just build off my branch you can do so to get those other changes in.

#5 Updated by Matt Robinson over 2 years ago

  • Status changed from Code Insufficient to Ready For Checkin
  • Assignee deleted (Sandor Szücs)

After talking with Sandor on the dev list I’m going to merge in my rework of his refactor patch.

#6 Updated by Matt Robinson over 2 years ago

  • Status changed from Ready For Checkin to Closed

Available in next branch commit:8e2a945368f8144947d8ada9542191f38358520b

#7 Updated by Jesse Wolfe over 2 years ago

Available in master as commit:d19f36e7e7d498b9ca12c5ce536f3ee3db114279 as part of Iteration-2010-11-17

Also available in: Atom PDF