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

Fix several bugs in slack_webhook returner #55968

Merged
merged 31 commits into from
Apr 23, 2020

Conversation

cdalvaro
Copy link
Contributor

@cdalvaro cdalvaro commented Jan 24, 2020

What does this PR do?

This PR fixes some issues detected by @max-arnold on #55342

What issues does this PR fix or reference?

Simple states like test.ping did not work with this returner.

GitHub-55968

Tests written?

👍

Commits signed with GPG?

👍

@cdalvaro cdalvaro requested a review from a team as a code owner January 24, 2020 20:17
@ghost ghost requested a review from Akm0d January 24, 2020 20:17
@max-arnold
Copy link
Contributor

@sagetherage Could you please label this for Neon? (this is a bugfix for a new feature that was merged to Neon)

@max-arnold
Copy link
Contributor

@cdalvaro Better, but is not quite there yet:

salt minion1 grains.items --return slack_webhook

2020-01-24 22:53:25,960 [salt.minion      :1819][ERROR   ][19444] The return failed for job 20200125065325865518: 'str' object has no attribute 'get'
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/salt/minion.py", line 1810, in _thread_return
    minion_instance.returners[returner_str](ret)
  File "/usr/lib/python3/dist-packages/salt/returners/slack_webhook_return.py", line 364, in returner
    report = _generate_report(ret, show_tasks)
  File "/usr/lib/python3/dist-packages/salt/returners/slack_webhook_return.py", line 306, in _generate_report
    returns = _process_returns(returns)
  File "/usr/lib/python3/dist-packages/salt/returners/slack_webhook_return.py", line 237, in _process_returns
    key=lambda s: s[1].get('__run_num__', 0)
  File "/usr/lib/python3/dist-packages/salt/returners/slack_webhook_return.py", line 237, in <lambda>
    key=lambda s: s[1].get('__run_num__', 0)
AttributeError: 'str' object has no attribute 'get'

I didn't test this thoroughly, but you can prevent the above traceback by adding the following function:

def _state_return(ret):
    '''
    Return True if ret is a Salt state return
    :param ret: The Salt return
    '''
    ret_data = ret.get('return')
    if not isinstance(ret_data, dict):
        return False

    return ret_data and '__id__' in next(iter(ret_data.values()))

And then replacing if isinstance(returns, dict): with if _state_return(ret):.
Please test it with other often used execution modules.

Also a small usability nitpick. Instead of:

webhook_url = 'https://hooks.slack.com/services/{}'.format(webhook)

Try this:

from salt.ext.six.moves.urllib.parse import urljoin as _urljoin

webhook_url = _urljoin('https://hooks.slack.com/services/', webhook)

This way specifying full webhook URLs would also work:

In [1]: from salt.ext.six.moves.urllib.parse import urljoin as _urljoin

In [2]: _urljoin('https://hooks.slack.com/services/', 'T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX')
Out[2]: 'https://hooks.slack.com/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX'

In [3]: _urljoin('https://hooks.slack.com/services/', 'https://hooks.slack.com/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX')
   ...:
Out[3]: 'https://hooks.slack.com/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX'

In [4]: _urljoin('https://hooks.slack.com/services/', 'https://whatever.slack.com/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX')
   ...:
Out[4]: 'https://whatever.slack.com/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX'

@cdalvaro
Copy link
Contributor Author

Thank you very much for all your help, @max-arnold

I start working on it right now!

@max-arnold
Copy link
Contributor

@cdalvaro AFAIK, there is no need to modify the man page. It will be automatically generated before the release.

@sagetherage
Copy link
Contributor

I have labeled @max-arnold but it needs to be rebased with Master and it needs to pass the build to get into Neon.

@cdalvaro cdalvaro changed the title WIP: Fix several bugs in slack_webhook returner Fix several bugs in slack_webhook returner Jan 26, 2020
@cdalvaro
Copy link
Contributor Author

cdalvaro commented Feb 5, 2020

All my tests are passing, but each time I merge the master branch with mine, distinct platforms/environments fail by different reasons.

@garethgreenaway
Copy link
Contributor

@cdalvaro I've restarted the tests that were failing and will monitor them to ensure we can get this merged.

@garethgreenaway garethgreenaway added ZRelease-Sodium retired label and removed ZRELEASED - Neon retired label labels Feb 14, 2020
@cdalvaro
Copy link
Contributor Author

@garethgreenaway, could you restart tests for ci/py2/centos6, it seems that failing tests are not related with my changes.

Thank you in advance!

@cdalvaro
Copy link
Contributor Author

test.ping @garethgreenaway

@cdalvaro
Copy link
Contributor Author

OMG! All checks have passed! 🎉

@Ch3LL Ch3LL removed the request for review from a team April 15, 2020 14:27
@dwoz dwoz merged commit ba3b379 into saltstack:master Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants