Skip to content

Fix executors not being called for multi-jobs#52939

Closed
andzn wants to merge 4 commits intosaltstack:2018.3from
andzn:dev/andzn/fix-executors-multi-jobs
Closed

Fix executors not being called for multi-jobs#52939
andzn wants to merge 4 commits intosaltstack:2018.3from
andzn:dev/andzn/fix-executors-multi-jobs

Conversation

@andzn
Copy link
Copy Markdown
Contributor

@andzn andzn commented May 8, 2019

What does this PR do?

Fixes a bug where custom executors were not being called for multi-jobs

What issues does this PR fix or reference?

Previous Behavior

Executors were not being called for multi-jobs

New Behavior

Executors are now called for multi-jobs as well as for single function jobs

Tests written?

No

Commits signed with GPG?

No

@andzn andzn requested a review from a team as a code owner May 8, 2019 11:41
Copy link
Copy Markdown
Contributor

@rares-pop rares-pop left a comment

Choose a reason for hiding this comment

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

Is there a test to 'patch' or can we write a new one to ensure things happens the way we expect, having custom executors set-up?

Comment thread salt/minion.py Outdated
Comment thread salt/minion.py

salt.utils.process.appendproctitle('{0}._thread_return {1}'.format(cls.__name__, data['jid']))

executors = data.get('module_executors') or opts.get('module_executors', ['direct_call'])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The setup for this function seems very similar if not identical with _thread_multi_return. Maybe it's worth refactoring that too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True. But this code was also touched in develop so I tried to keep the changes to a minimum so that the merging to 2019.2 would not be a head-ache.

@andzn
Copy link
Copy Markdown
Contributor Author

andzn commented May 8, 2019

Is there a test to 'patch' or can we write a new one to ensure things happens the way we expect, having custom executors set-up?

#52773 is adding an executor integration test. I can add a new test there once that gets in. However that's only in the 2019.2 branch.

@mattp-
Copy link
Copy Markdown
Contributor

mattp- commented May 8, 2019

👍 nice add

Copy link
Copy Markdown
Contributor

@rares-pop rares-pop left a comment

Choose a reason for hiding this comment

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

LGTM

@andzn
Copy link
Copy Markdown
Contributor Author

andzn commented Jul 12, 2019

I created a new PR #53844 based on the 2019.2 branch that also contains a test.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants