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

[2018.3] Fix to cmd_batch #48236

Merged
merged 6 commits into from Jun 25, 2018

Conversation

Projects
None yet
4 participants
@garethgreenaway
Member

garethgreenaway commented Jun 21, 2018

What does this PR do?

Fixing cmd_batch to work correctly when called via salt-api.

What issues does this PR fix or reference?

#48141

Previous Behavior

When running from the salt-api this would result in an exception, presumably because salt.utils.versions was unavailable.

New Behavior

import salt.utils.versions inside cmd_batch.

Tests written?

Yes

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@garethgreenaway garethgreenaway requested a review from saltstack/team-core as a code owner Jun 21, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Jun 21, 2018

@isbm

This is really weird. The module is imported and is used all over the place. And only that one function fails?? How come?

@gtmanfred

This comment has been minimized.

Contributor

gtmanfred commented Jun 21, 2018

Probably because of multiprocessing

@@ -559,6 +559,7 @@ def cmd_batch(
{'stewart': {...}}
'''
if 'expr_form' in kwargs:
import salt.utils.versions

This comment has been minimized.

@rallytime

rallytime Jun 21, 2018

Contributor

Can you add a comment here about why this later import is needed so we don't accidentally remove it in the future?

@garethgreenaway garethgreenaway force-pushed the garethgreenaway:48141_salt_api_local_batch branch from 95c4133 to 335b5e1 Jun 21, 2018

@rallytime

And a test to boot! Well done.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 22, 2018

@garethgreenaway That test is failing, however. Can you take a look?

garethgreenaway added some commits Jun 21, 2018

@garethgreenaway garethgreenaway force-pushed the garethgreenaway:48141_salt_api_local_batch branch from 9baed5b to 83accf3 Jun 22, 2018

garethgreenaway added some commits Jun 23, 2018

@isbm

isbm approved these changes Jun 25, 2018

@garethgreenaway thanks for the explanation comment.

# We need to re-import salt.utils.versions here
# even though it has already been imported.
# when cmd_batch is called via the NetAPI
# the module is unavailable.

This comment has been minimized.

@isbm

isbm Jun 25, 2018

Contributor

👍

@rallytime rallytime merged commit 6ee8566 into saltstack:2018.3 Jun 25, 2018

7 of 9 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5982 — ABORTED
Details
default Build finished.
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #26186 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #18236 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23910 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10952 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22871 — SUCCESS
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #20035 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment