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

[2017.7] reset file_roots for renderers after compile_pillar #48388

Merged
merged 3 commits into from Jul 11, 2018

Conversation

Projects
None yet
5 participants
@garethgreenaway
Member

garethgreenaway commented Jun 29, 2018

What does this PR do?

When pillar items are compiled a new render is instantiated but the file_roots is the pillar_roots. This change forces the opts['file_roots'] to be set to what is set in actual_file_roots for all renderers once compile_pillar has finished. Adding a test when this situation is run via a orchestration runner.

What issues does this PR fix or reference?

#48277
#46986

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 29, 2018

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

@garethgreenaway garethgreenaway requested a review from terminalmage Jun 29, 2018

@garethgreenaway garethgreenaway force-pushed the garethgreenaway:48277_2017_7_file_roots_wrong branch from fb23106 to 386fade Jun 30, 2018

__reload_config=True).get('jid')
if jid is None:
raise Exception('jid missing from run_run_plus output')

This comment has been minimized.

@isbm

isbm Jul 2, 2018

Contributor

Why do we raise common exceptions, instead of AssertionError?

assert bool(jid), 'Job ID missing from run_run_plus output'

This comment has been minimized.

@gtmanfred

gtmanfred Jul 2, 2018

Contributor

If you do an assert, it gets removed when the optomized bytecode is compiled at build time.

https://stackoverflow.com/questions/9097706/why-assert-is-not-largely-used

This comment has been minimized.

@isbm

isbm Jul 2, 2018

Contributor

Oh, right, my bad. Well, I use so often optimised mode that I totally forgot about that (last 20 years of Python and none of it at all). 😉

OK, how about raise other than generic Exception?

event = listener.get_event(full=True)
if event is None:
continue
if event['tag'] == 'salt/run/{0}/ret'.format(jid):

This comment has been minimized.

@isbm

isbm Jul 2, 2018

Contributor

I'd do it more safe:

if event.get('salt/run/{0}/ret'.format(jid)):
    ...

This comment has been minimized.

@garethgreenaway

garethgreenaway Jul 2, 2018

Member

This would trigger the try...finally as the key in the event dictionary is tag not the tag itself.
Something like this would work though:

if event.get('tag', '') == 'salt/run/{0}/ret'.format(jid):
break
finally:
self.assertTrue(received)
del listener

This comment has been minimized.

@isbm

isbm Jul 2, 2018

Contributor

BTW, is this really needed?

This comment has been minimized.

@garethgreenaway

garethgreenaway Jul 2, 2018

Member

Which part specifically? The assertTrue or the clean up of the listener?

This comment has been minimized.

@isbm

isbm Jul 3, 2018

Contributor

The del. Seems you're basically calling SaltEvent.__del__ in order to call SaltEvent.destroy. I would say, we should not do this, because __del__ is not an opposite to __init__. I would rather refactor it a bit:

class SaltEvent(event):
    def destroy(self):
        try:
            if self.subscriber is not None:
                self.subscriber.close()
            if self.pusher is not None:
                self.pusher.close()
            if self._run_io_loop_sync and not self.keep_loop:
                self.io_loop.close()
        except Exception as err:
            log.error('Unable to close event listener')
            log.debug(err)

    __del__ = destroy

And then we should never use it as del obj but always obj.destroy() instead. However, we still leave __del__ to the garbage collector.

Can we do this, please?

garethgreenaway added some commits Jun 29, 2018

When pillar items are compiled a new render is instantiated but the f…
…ile_roots is the pillar_roots. This change forces the __opts__['file_roots'] to be set to what is set in actual_file_roots for all renderers once compile_pillar has finished. Adding a test when this situation is run via a orchestration runner.

@garethgreenaway garethgreenaway force-pushed the garethgreenaway:48277_2017_7_file_roots_wrong branch from 386fade to 6f11da3 Jul 2, 2018

@rallytime rallytime merged commit ad5a959 into saltstack:2017.7 Jul 11, 2018

5 of 10 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #6188 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #11158 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #20241 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #26399 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #18436 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #24116 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #23071 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment