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

detect running from master in State.event method #34256

Merged

Conversation

tmehlinger
Copy link
Contributor

@tmehlinger tmehlinger commented Jun 23, 2016

What does this PR do?

Fixes (I hope) State.event to work correctly when running on the master. This will enable firing events using the fire_eventrequisite in orchestration states.

I've tested this with the test case I provided on #34255 and it works as I expect; it produces the following event:

salt/state_result/my.master_master/my/event    {
    "_stamp": "2016-06-23T21:00:08.733246",
    "ret": {
        "__id__": "testing",
        "__run_num__": 0,
        "changes": {},
        "comment": "Success!",
        "duration": 0.328,
        "name": "test.ping",
        "result": true,
        "start_time": "21:00:08.731032"
    }
}

What issues does this PR fix or reference?

#34255

Previous Behavior

fire_event requisite did not work with orchestration.

New Behavior

fire_event requisite works with orchestration. Does not appear to utterly destroy everything else that depends on State.fire_event.

Tests written?

No -- looking for guidance.

Please review Salt's Contributing Guide for best practices.

This changes State.event to detect whether the state that invoked fire_event is running on the master. This fixes an issue where the fire_event requisite doesn't work when being used in orchestration states.

This changes `State.event` to detect whether the state that invoked `fire_event` is running on the master. This fixes an issue where the `fire_event` requisite doesn't work when being used in orchestration states.
@@ -1936,7 +1936,13 @@ def event(self, chunk_ret, length, fire_event=False):
chunk is evaluated an event will be set up to the master with the
results.
'''
if not self.opts.get('local') and (self.opts.get('state_events', True) or fire_event) and self.opts.get('master_uri'):
if not self.opts.get('local') and (self.opts.get('state_events', True) or fire_event):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm somewhat doubtful that this and the following conditions are sufficient for determining whether the state is being applied on the master or the minion. I'm looking for insight on whether this is a good change or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this approach will work.

@cachedout
Copy link
Contributor

@tmehlinger This looks good to me. Just some minor lint issues for you to fix up and then I'll get this in. https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/2849/violations/file/salt/state.py/

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jun 24, 2016
@tmehlinger
Copy link
Contributor Author

Linting issues resolved!

Any chance this would make it into the next release? It would help me a ton.

pleeeaase

@cachedout cachedout added the bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch label Jun 27, 2016
@cachedout
Copy link
Contributor

Go Go Jenkins!

@cachedout cachedout merged commit 31de61e into saltstack:develop Jun 28, 2016
@cachedout
Copy link
Contributor

@tmehlinger I marked it as a backport for you. Thanks!

@rallytime rallytime added ZZZ[Done]-back-ported-bf RETIRED The pull request has been back-ported to an older branch. and removed bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged labels Jun 28, 2016
rallytime pushed a commit that referenced this pull request Jun 28, 2016
* detect running from master in State.event method

This changes `State.event` to detect whether the state that invoked `fire_event` is running on the master. This fixes an issue where the `fire_event` requisite doesn't work when being used in orchestration states.

* fix linting issues
gitebra pushed a commit to gitebra/salt that referenced this pull request Jun 29, 2016
* upstream/develop: (44 commits)
  Send traps to Zabbix when there is a comment and name in result.
  Fix invalid syntax error in system.py module exception handling
  Don't forget the pylint disables for range
  Back-port saltstack#34256 to 2016.3 (saltstack#34343)
  Pylint fixes
  Revert py3modernize lint changes (saltstack#34339)
  Add listen/listen_in support to stateconf.py
  Change merge-if-exists logic to properly report changes
  Doc clarifications to file modules, addition of new `profile` log level to docs, fixed example in dnsmasq (saltstack#34323)
  version checking had a logical error (saltstack#34333)
  fix saltstack#34329 (saltstack#34330)
  Remove unnecessarily-disabled sanity check (saltstack#34325)
  Do not force 'filter' table when flushing
  add osmajorrelease to ubuntu and fix pylint
  Fix psutil.cpu_times unpack error (saltstack#34318)
  Typo in dockerio doc (saltstack#34319)
  add osmajorrelease grain for raspbian
  [2016.3] Update to latest bootstrap script v2016.06.27
  [2015.8] Update to latest bootstrap script v2016.06.27
  [2015.5] Update to latest bootstrap script v2016.06.27
  ...
@tmehlinger tmehlinger deleted the master-fire-event-orchestration branch August 25, 2016 23:32
@tmehlinger tmehlinger mentioned this pull request Aug 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZZZ[Done]-back-ported-bf RETIRED The pull request has been back-ported to an older branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants