Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pin munki-facts to munkipython and remove all pre 3.6 deprecated code #17

Merged
merged 3 commits into from
Oct 13, 2020

Conversation

erikng
Copy link
Contributor

@erikng erikng commented Feb 14, 2020

I've reworked all of the munki-facts code to stop supporting deprecated modules

DeprecationWarning: The readPlist function is deprecated, use load() instead

DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses

DeprecationWarning: The writePlist function is deprecated, use dump() instead

This will mean that munki-facts will only support python 3.6 and higher, but this will also use munki 4.0's built in python, which uses python 3.7. I think given the upcoming changes to macOS, it would be prudent to make this breaking change now and give people a migration path.

@erikng erikng requested a review from gregneagle February 14, 2020 19:22
@erikng
Copy link
Contributor Author

erikng commented May 26, 2020

Ping on this @gregneagle

@gregneagle
Copy link
Contributor

So we need to communicate widely that this change will break any munki-facts you happen to deploy that are not Python-3 compatible.

@gregneagle
Copy link
Contributor

Are all the included facts Python 3-ready?

@erikng
Copy link
Contributor Author

erikng commented Jun 4, 2020

Good call. I just tested them and 3 had issues. They are now fixed.

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
	<key>admin_users</key>
	<array>
		<string>root</string>
		<string>something</string>
	</array>
	<key>backtomymac_configured</key>
	<false/>
	<key>catalina_upgrade_supported</key>
	<false/>
	<key>crashplan_username</key>
	<string>something</string>
	<key>filevault_status</key>
	<string>FileVault is On.</string>
	<key>gatekeeper_status</key>
	<string>assessments enabled</string>
	<key>local_user_dirs</key>
	<array>
		<string>something</string>
	</array>
	<key>mojave_upgrade_supported</key>
	<false/>
	<key>physical_or_virtual</key>
	<string>physical</string>
	<key>sip_status</key>
	<string>System Integrity Protection status: enabled.</string>
</dict>
</plist>

The catalina and mojave scripts are out of date, but I don't think it's relevant to fix them for this PR.

@erikng
Copy link
Contributor Author

erikng commented Jul 1, 2020

@gregneagle any thoughts here?

@gregneagle
Copy link
Contributor

The shebang line will need to change for Munki 5.1, so let’s make that change now and then merge this after Munki 5.1 ships.

@erikng
Copy link
Contributor Author

erikng commented Jul 25, 2020

Done.

@natewalck
Copy link

🏓

@gregneagle
Copy link
Contributor

As I mentioned before, I'll merge this after the Munki 5.1 release.

@natewalck
Copy link

Disregard, I forgot. 🤣 I saw an internal PR and pinged this as a result.

@erikng
Copy link
Contributor Author

erikng commented Jul 28, 2020

Let it be known that this internal PR stated this very fact about 5.1.

@natewalck
Copy link

Let it be known I don't read your comments 🤣

Copy link

@natewalck natewalck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5.1 Munki path changed back.

@@ -1,13 +1,13 @@
#!/usr/bin/python
#!/usr/local/munki/munki-python
Copy link

@natewalck natewalck Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be #!/usr/local/munki/python now I believe.

Copy link
Contributor

@gregneagle gregneagle Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not that!

(and we're back to /usr/local/munki/munki-python...)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make up our minds! 🤣

Copy link

@natewalck natewalck Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated, DISREGARD THIS :D It's fine again.

@erikng
Copy link
Contributor Author

erikng commented Oct 13, 2020

@gregneagle what do I need to do here to get this merged?

@gregneagle gregneagle merged commit e6ed4a1 into munki:master Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants