Add pre-commit configuration that catches common issues with preference manifests

[homebysix]

Summary

This PR adds a configuration used by the pre-commit framework, meant for catching common issues before they get committed and pushed. The configured check-preference-manifests hook is hosted in my pre-commit-macadmin repository, and the source code for the hook can be found here.

Installation

Upon merging this PR, frequent contributors should be encouraged to do the following in order to use the pre-commit configuration.

1. Install pre-commit.
2. Activate the hooks within this repo:

   cd ~/src/ProfileManifests
   pre-commit install
   

From that point on, every commit will be checked automatically.

The hooks will not run unless the above actions are taken, so this PR will not affect automated processes or the GitHub Actions configuration.

Benefits

I've found pre-commit to be extremely helpful when managing repositories with large numbers of files in the same format (e.g. AutoPkg recipes, Munki pkginfo files, Python files, etc). I gave an introduction to pre-commit at this 2019 PSU MacAdmins session and in this post.

Here are a few examples of errors that my pre-commit hook can catch:

Plist syntax issues

A manifest with this syntax error:

<key>pfm_description</string>
<string>Configures available Share menu options</string>

Will produce this error when a commit attempt is made:

% git commit -m "test commit"
Check Apple Preference Manifests.........................................Failed
- hook id: check-preference-manifests
- exit code: 1

Manifests/ManifestsApple/com.apple.ShareKitHelper.plist: plist parsing error: mismatched tag: line 5, column 23

Missing required keys

A manifest with missing title, domain, or description keys at the top level:

<key>pfm_domain</key>
<string></string>

Will produce this error when a commit attempt is made:

% git commit -m "test commit"
Check Apple Preference Manifests.........................................Failed
- hook id: check-preference-manifests
- exit code: 1

Manifests/ManagedPreferencesApplications/com.apple.logic10.plist: <root dict> missing required key pfm_domain

Incorrect manifest format version

A manifest with pfm_format_version other than 1:

<key>pfm_format_version</key>
<integer>3</integer>

Will produce this error when a commit attempt is made:

% git commit -m "test commit"
Check Apple Preference Manifests.........................................Failed
- hook id: check-preference-manifests
- exit code: 1

Manifests/ManagedPreferencesApplications/com.apple.logic10.plist: pfm_format_version should be 1, not 3 (https://github.com/ProfileCreator/ProfileManifests/wiki/Manifest-Format-Versions)

Incorrect key types

A manifest with an incorrect key type:

<key>pfm_platforms</key>
<string>macOS</string>

Will produce this error when a commit attempt is made:

% git commit -m "test commit"
Check Apple Preference Manifests.........................................Failed
- hook id: check-preference-manifests
- exit code: 1

Manifests/ManagedPreferencesApplications/com.barebones.bbedit.plist: manifest key pfm_platforms should be type <class 'list'>, not type <class 'str'>

Incorrect list item types

A manifest with an incorrectly typed list item:

<key>pfm_range_list_titles</key>
<array>
	<integer>0</integer>
	<integer>1</integer>
	<integer>5</integer>
	<integer>10</integer>
	<integer>50</integer>
	<integer>100</integer>
	<integer>500</integer>
</array>

Will produce this error when a commit attempt is made:

% git commit -m "test commit"
Check Apple Preference Manifests.........................................Failed
- hook id: check-preference-manifests
- exit code: 1

Manifests/ManagedPreferencesApple/com.apple.safari.plist: "pfm_range_list_titles" items should be type <class 'str'>, not type <class 'int'>

Default value doesn't match setting type

A manifest with an incorrectly typed default item:

<dict>
	<key>pfm_name</key>
	<string>TMAFirstLaunchVersion</string>
	<key>pfm_type</key>
	<string>integer</string>
	<key>pfm_default</key>
	<false />
</dict>

Will produce this error when a commit attempt is made:

% git commit -m "test commit"
Check Apple Preference Manifests.........................................Failed
- hook id: check-preference-manifests
- exit code: 1

Manifests/ManagedPreferencesApple/com.apple.iWork.Numbers.plist: pfm_default value for TMAFirstLaunchVersion should be type <class 'int'>, not type <class 'bool'>

Successful check output

When a commit is done and all tests pass, the output will look similar to this:

% git commit -m "test commit" testfile.plist
Check Apple Preference Manifests.........................................Passed
[testbranch 66c33bc] test commit
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 testfile.plist

Forcing a check of all files

For troubleshooting or quality control purposes, you can initiate a check of all manifests using pre-commit run --all-files.

Opting out per-commit

If you need to intentionally bypass the checks for a single commit, you can use git commit -n or git commit --no-verify.

Hook contributions welcome

My pre-commit-macadmin repo is still evolving, and I'd love feedback and/or contributions if anybody is interested in adding more tests that manifest files should pass in order to be committed.

[relgit]

Elliot, this is great; lots of ground covered there.

Would you mind if we embed check_preference_manifests.py (which is the bulk of your work here) locally and use Git's native pre-commit hook instead of the pre-commit utility?

I'm finding it hard getting over the script being on a separate repository, both from security and flexibility standpoints.

What do you think?

[homebysix]

I wouldn't recommend that, for two reasons:

• The security concerns (which are totally justifiable) should be covered by the fact that pre-commit pins to a specific rev of the hook. Currently that's v1.12.1, and any changes would need to be manually updated (and approved) by the repo maintainers. (Similar to trust info in AutoPkg recipes or version pinning in Python requirements files.)

• There are a number of other pre-commit hooks which may be valuable to this repo, and copying their code in manually isn't scalable (or permissible by some licenses, I would think).

The pre-commit framework itself does all the work of checking out the correct repositories at the correct rev for each hook, so the fact that the hook is in a different repo will be invisible to contributors.

[apizz]

@relgit are you OK with the changes here given the most recent comments?

[relgit]

Tags can be reassigned so I'd like to see rev changed to use frozen commit hashes before I can comfortably give my thumbs-up.

Also, I have two more questions because otherwise that would be too easy -

• What other hooks for example?
• The PFM spec is pretty much set in stone by now, but we of course use many unofficial extensions. What do we do when we want to add more or modify existing?

[homebysix]

I adjusted the PR to use a frozen commit hash per your request: bd607b5

Other hooks I've found beneficial for this type of repository (which includes primarily plist and python files) would include:

• From https://github.com/pre-commit/pre-commit-hooks:
  • check-ast: ensures Python files compile
  • check-case-conflict: ensures no files are committed that would cause problems on case insensitive filesystems
  • check-executables-have-shebangs: ensures proper use of executable file mode
  • check-merge-conflict: prevents committing files containing unresolved merge conflicts
  • end-of-file-fixer: ensures consistent EOFs
  • fix-encoding-pragma: adds encoding line to Python files
  • trailing-whitespace: ensures no extraneous whitespace at line endings, therefore simplifying diffs from people whose editors automatically remove this whitespace
• From https://github.com/ambv/black:
  • black: applies Black format to Python files
• From https://gitlab.com/pycqa/flake8:
  • flake8: Python PEP8 compliance
• From https://github.com/PyCQA/pylint:
  • pylint: Python linting and bug catching

As the maintainer of the pre-commit-macadmin repo I'll be open to PRs adding checks for unofficial extensions or modifying existing checks. For the current hook, I've focused the checks around the most common issues I've seen in the wild (and made myself).

[homebysix]

Also worth saying: I'm not aware of any other large repository of preference manifest files, so feedback from the team maintaining this repo will be weighted heavily.

Additional checks for the check-preference-manifests hook would likely be added somewhere in the validate_subkeys() function. Simple checks can be done inline (example), and more complex checks can have their own function (example).

[homebysix]

After thinking it over, here's an alternative: you could create a new repository containing this pre-commit hook within the ProfileCreator GitHub organization. I'd be happy to contribute the necessary code to get started, but then you'd be on the hook for maintaining and fielding PRs (many probably from me). Does that appeal to you?

[apizz]

@homebysix Know you're a busy guy, but wondered if a short web call might help clear up the desired changes and ensure that works for all contributing parties. I know I personally greatly appreciate your efforts here!

[homebysix]

Sure, I'm up for that! Let's coordinate on Slack.

[relgit]

@homebysix the first option seems satisfactory to me especially now that rev is frozen, so thanks for elaborating and for making that change.

You've got my 👍

[apizz]

Since @relgit 's concerns were addressed, I'm going to update our documentation for contributing to indicate how this is to be used. Once that's done and you give me 👍 on it @homebysix I'll merge this in.

Thanks again for this!

[apizz]

Let me know what you think: https://github.com/ProfileCreator/ProfileManifests/wiki/Contribute-to-an-Existing-Manifest

[apizz]

I was testing how I could break the pre-commit hooks ... very impressed :)

One thing I did discover is that while it will fail to let me commit if I remove the pfm_type key it will notably commit just fine without a pfm_name key. It's required for top-level keys (first layer of pfm_subkeys) but not for all items in the layer below that (thinking of an array with a subkey that's a string). Not sure how difficult it would be to add this just at the layer we need, but if we could get that included as a required key that would be awesome!

[apizz]

@homebysix Sorry meant to tag you on ^^

[homebysix]

That's a worthwhile adjustment. I've created an issue here to track progress: homebysix/pre-commit-macadmin#54

[homebysix]

I've added the requested pfm_type and pfm_name checks into v1.12.2 of the pre-commit hooks, and updated (2bd2b4a) the commit hash this PR is pointing to accordingly.

For testing, I'll use this section of the MunkiReport.plist manifest, which includes pfm_subkeys that have pfm_type dictionary.

<dict>
	<key>pfm_description</key>
	<string>ReportItems contains a dictionary that tells MunkiReport where to look for the report files. The key is the name of the module, the value contains a path to the appropriate file. Note: ReportItems are set automatically when installing MunkiReport via the command line or a package.</string>
	<key>pfm_documentation_url</key>
	<string>https://github.com/munkireport/munkireport-php/wiki/Client-Configuration#reportitems</string>
	<key>pfm_name</key>
	<string>ReportItems</string>
	<key>pfm_subkeys</key>
	<array>
		<dict>
			<key>pfm_description</key>
			<string></string>
			<key>pfm_name</key>
			<string>{{key}}</string>
			<key>pfm_title</key>
			<string>MunkiReport Module</string>
			<key>pfm_type</key>
			<string>string</string>
			<key>pfm_value_placeholder</key>
			<string>ex. localadmin</string>
		</dict>
		<dict>
			<key>pfm_description</key>
			<string></string>
			<key>pfm_name</key>
			<string>{{value}}</string>
			<key>pfm_title</key>
			<string>File Path</string>
			<key>pfm_type</key>
			<string>string</string>
			<key>pfm_value_placeholder</key>
			<string>ex. /usr/local/munki/preflight.d/cache/localadmins.txt</string>
		</dict>
	</array>
	<key>pfm_type</key>
	<string>dictionary</string>
</dict>

First, I removed the pfm_name from one of the subkeys:

diff --git a/Manifests/ManagedPreferencesApplications/MunkiReport.plist b/Manifests/ManagedPreferencesApplications/MunkiReport.plist
index 8b0d588..8e81a59 100644
--- a/Manifests/ManagedPreferencesApplications/MunkiReport.plist
+++ b/Manifests/ManagedPreferencesApplications/MunkiReport.plist
@@ -140,8 +140,6 @@
 				<dict>
 					<key>pfm_description</key>
 					<string></string>
-					<key>pfm_name</key>
-					<string>{{key}}</string>
 					<key>pfm_title</key>
 					<string>MunkiReport Module</string>
 					<key>pfm_type</key>

The hook fails as expected:

% pre-commit run --all-files
Check Apple Preference Manifests.........................................Failed
- hook id: check-preference-manifests
- exit code: 1

Manifests/ManagedPreferencesApplications/MunkiReport.plist: ReportItems subkey missing required key pfm_name

And if instead I remove the pfm_type:

diff --git a/Manifests/ManagedPreferencesApplications/MunkiReport.plist b/Manifests/ManagedPreferencesApplications/MunkiReport.plist
index 8b0d588..ea2e7a8 100644
--- a/Manifests/ManagedPreferencesApplications/MunkiReport.plist
+++ b/Manifests/ManagedPreferencesApplications/MunkiReport.plist
@@ -144,8 +144,6 @@
 					<string>{{key}}</string>
 					<key>pfm_title</key>
 					<string>MunkiReport Module</string>
-					<key>pfm_type</key>
-					<string>string</string>
 					<key>pfm_value_placeholder</key>
 					<string>ex. localadmin</string>
 				</dict>

The hook also fails as expected. (Note that {{key}} is the actual name of the key here.)

% pre-commit run --all-files
Check Apple Preference Manifests.........................................Failed
- hook id: check-preference-manifests
- exit code: 1

Manifests/ManagedPreferencesApplications/MunkiReport.plist: {{key}} missing required key pfm_type

If I put both pfm_name and pfm_type back, the hook succeeds as expected:

% pre-commit run --all-files                                                                                                        
Check Apple Preference Manifests.........................................Passed

[apizz]

Looks great! Can you review the wiki page I updated around the pre-commit install steps for contributing? If @relgit is ok with this will get this merged.

[apizz]

Thanks so much for all your work on this, @homebysix :)

[relgit]

Yes, this looks good.