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

add macOS mac_service update. #47256

Merged
merged 8 commits into from May 22, 2018

Conversation

Projects
None yet
4 participants
@weswhet
Copy link
Contributor

commented Apr 23, 2018

What does this PR do?

This PR adds support for using the latest macOS launchctl subcommands as well as the ability to manage user specific services.

Previous Behavior

Previously salt would use the legacy subcommands like /bin/launchctl load/unload to start and stop services. It was also not capable of modifying any user specific services located in /Users/foo/Library/LaunchAgents/

New Behavior

Salt now leverages the latest subcommands /bin/launchctl bootstrap/bootout to start and stop services. Using these subcommands allows us to properly load and unload LauchAgents from root without having to run commands as the user as this is handled properly by launchctl when specifying to appropriate domain/system targets like gui/501/ or gui/501/com.apple.example.

Salt also now looks for user specific services located in /Users/foo/Library/LaunchAgents/ and is now capable of modifying them.

Tests written?

No

Commits signed with GPG?

Yes

add in support for the latest mac_service launchctl subcommands inste…
…ad of the legacy versions, and support for user space LaunchAgents

@rallytime rallytime requested review from twangboy and gtmanfred Apr 24, 2018

@gtmanfred
Copy link
Contributor

left a comment

Just one change I would like to see, but it can be merged without it.

# check if a LaunchAgent as we should treat these differently.
if 'LaunchAgents' in path:
# Get the console user so we can service in the correct session
uid = salt.utils.mac_utils.console_user()

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Apr 24, 2018

Contributor

can you switch these to using __utils__['mac_utils.console_user']() instead of using the import salt.utils.mac_utils?

It allows for people that use the dropin dynamic modules to actually replace the modules that use newer salt utils.

Thanks,
Daniel

This comment has been minimized.

Copy link
@weswhet

weswhet Apr 24, 2018

Author Contributor

@gtmanfred would like me to convert the other mac_util calls as well?

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Apr 24, 2018

Contributor

Eventually we should go through and change them all to using __utils__ i was just think about these ones since you are adding new ones.

This comment has been minimized.

Copy link
@weswhet

weswhet Apr 24, 2018

Author Contributor

Sounds good, I changed all my additions to using __utils__

This comment has been minimized.

Copy link
@twangboy

twangboy May 1, 2018

Contributor

I'm still seeing salt.utils.mac_utils.console_user(). Need change it to __utils__['mac_utils.console_user']

making requested changes and adding in a patch to fix launchagents wi…
…th PIDs as the service state does not pass in the runas parameter
@weswhet

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

after testing the requested changes by Daniel, I noticed that the service state doesn't pass in the runas parameter when checking the status of a LaunchAgent. I've added a patch for this behavior in ea6c18c

@weswhet

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@rallytime I'd like to update the documentation with some notes and version added information for these changes. Would this PR make it into the Fluorine release?

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2018

@weswhet Yeah, definitely. We're not branching for the Fluorine release for a while.

weswhet and others added some commits Apr 26, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

Hi @weswhet - It looks like this change is causing the following related tests to fail:

  • unit.utils.test_mac_utils.MacUtilsTestCase.test_available_services
  • unit.utils.test_mac_utils.MacUtilsTestCase.test_available_services_broken_symlink
  • unit.utils.test_mac_utils.MacUtilsTestCase.test_available_services_non_xml
  • unit.utils.test_mac_utils.MacUtilsTestCase.test_available_services_non_xml_malformed_plist

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/22418/

Can you take a look?

@weswhet

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2018

@rallytime, for future reference what state are the test Macs in? I’m assuming headless With no users and sitting at the login window? Just curious for any future code changes.

weswhet added some commits May 1, 2018

@rallytime rallytime merged commit de84f6f into saltstack:develop May 22, 2018

5 of 10 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #4571 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #9495 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #18623 — ABORTED
Details
codeclimate 6 issues to fix
Details
default Build finished.
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #24743 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #16871 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #22450 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21491 — SUCCESS
Details

@weswhet weswhet deleted the weswhet:add-mac_service-update branch Sep 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.