ProfileCreator / ProfileManifests

Manifest repository for the ProfilePayloads framework
345 stars 146 forks source link

Standardization #609

Closed relgit closed 1 year ago

relgit commented 1 year ago

This PR comprises of @DigiYann's and my proposal on how to clean up and standardize ProfileManifests, in-line with discussions on MacAdmins and in #567.

Essentially, the PR removes all XML comments, truncates whitespace, and introduces pfmx_comment for per-key comments. This was done by a combination of running the files through plutil as well as several manual changes.

A number of keys that were previously commented-out have been brought out to serve as actual keys in their respective manifests, in consultation with their domain documentation where applicable.

We are aware that merging this may lead to conflicts with pending PRs. However, those can be easily resolved by requesting that all pending PRs be passed through plutil as well before getting merged.

kevinmcox commented 1 year ago

@relgit Are there any downsides you can think of to making this change? Have they been tested with both iMazing and ProfileCreator?

relgit commented 1 year ago

Hey all, thanks for your comments and questions. We were able to turn our attention back to finalizing this PR over the last two weeks, and I believe it is now ready to be merged.

@homebysix we've now added any manifest that could benefit from standardization, bringing the changed manifest tally up to 113.

@kevinmcox no real downsides, and yes, we were able to test the work on both iMazing Profile Editor and ProfileCreator. No issues were encountered on either.

A number of other notes:

When we encountered commented-out preference keys, we looked them up in their respective domain documentation. Those that we could find were uncommented (as suggested in https://github.com/ProfileCreator/ProfileManifests/issues/567#issuecomment-1274874039), and those that we couldn't got removed.

ProfileCreator developer-mode manifests were not included.

And lastly, there is a real-number precision issue that we observed, where plutil will write real numbers as their nearest double-representable neighbor (for example 0.1 turns into 0.10000000000000001). In testing this made no difference for things like range validation, but it is semantically inaccurate and visually unappealing.

Due to these, we decided to keep the numbers as they were. If in the future this gets in the way of diffing or automating changes, we can always change, or create a small utility to standardize real numbers specifically.

By the way, this behavior is not limited to just plutil. PlistBuddy and (admittedly) the macOS iPE also behave similarly, probably because all three use the same CoreFoundation code to handle plists. Interestingly, Xcode's plist editor does not suffer from this.

Well, that's it for now. I invite you to look at the changes that we've made and even test the PR if you like. We're planning more work on ProfileManifests so it would be nice to be able to merge this and close #567 soon.

kevinmcox commented 1 year ago

@relgit thanks for all those details.

apizz commented 1 year ago

@homebysix I think the only thing that we should take in account before this gets merged in is how pfmx_comment is processed in https://github.com/Jamf-Custom-Profile-Schemas/ProfileManifestsMirror.

apizz commented 1 year ago

Think I was able to address in https://github.com/Jamf-Custom-Profile-Schemas/ProfileManifestsMirror/pull/22, but I'll let @homebysix confirm

relgit commented 1 year ago

I was able to confirm locally that the changes have no negative effects on ProfileManifestsMirror so we can safely merge.

@kevinmcox do we have your OK?