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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix memoize on available macOS services #49062

Merged
merged 2 commits into from Aug 15, 2018

Conversation

Projects
None yet
4 participants
@weswhet
Contributor

weswhet commented Aug 10, 2018

What does this PR do?

Changes macOS services to using the context dictionary versus the memoize decorator. And some other pylint fixes. Would love to get this into 2018.3.3 if possible 馃槂

What issues does this PR fix or reference?

#48414

Previous Behavior

Previously on the macOS services module, it would use the memoize decorator to capture available services. This was causing an issue with newly created services. See the referenced issue for more details.

New Behavior

Now uses the __context__ dictionary, to avoid the previous issue and still maintain efficient runtimes.

Tests written?

No

Commits signed with GPG?

Yes

cc @gtmanfred @terminalmage as they recommended this fix.

@cachedout cachedout requested a review from Ch3LL Aug 10, 2018

@cachedout

This comment has been minimized.

Contributor

cachedout commented Aug 10, 2018

@Ch3LL Can you take the lead on reviewing this, please?

# we received a cached batch of services, if not we did a fresh check
# so we need to raise that the service could not be found.
try:
if not __context__['using_cached_services']:

This comment has been minimized.

@Ch3LL

Ch3LL Aug 14, 2018

Contributor

i'm not seeing using_cached_services anywhere else in the code. Did you mean for that to be available_services_cached possibly or maybe i'm not reading it correctly?

This comment has been minimized.

@weswhet

weswhet Aug 14, 2018

Contributor

Good catch, I changed it to be somewhat more human readable and missed that one.

This comment has been minimized.

@weswhet

weswhet Aug 14, 2018

Contributor

updated with the most recent commit.

@Ch3LL

This comment has been minimized.

Contributor

Ch3LL commented Aug 14, 2018

there are related test failures here: https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py2/job/PR-49062/1/

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

@rallytime

This comment has been minimized.

Contributor

rallytime commented Aug 14, 2018

@weswhet

This comment has been minimized.

Contributor

weswhet commented Aug 14, 2018

Requested changes have been made and I believe I figured out the Tests bit. 馃

@cachedout

This comment has been minimized.

Contributor

cachedout commented Aug 15, 2018

@Ch3LL Re-review requested.

@Ch3LL

Ch3LL approved these changes Aug 15, 2018

@cachedout cachedout merged commit c8510a6 into saltstack:2018.3 Aug 15, 2018

5 of 8 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
@Ch3LL

This comment has been minimized.

Contributor

Ch3LL commented Aug 15, 2018

@weswhet i'm adding your PR to the 2018.3.3 branch here: #49132 so we can get it in for the 2018.3.3 release :)

cachedout added a commit that referenced this pull request Aug 17, 2018

@weswhet weswhet deleted the weswhet:fix-mac-available-services branch Sep 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment