Add notify-errbot rule to ChatOps pack. by nzlosh · Pull Request #5051 · StackStorm/st2

nzlosh · 2020-10-02T11:35:51Z

Add notify-errbot rule to chatops pack to simplify installation for err-stackstorm users. (transpartent for non err-stackstorm users).

Updated readme using pack2md as well as fix file permissions and the metadata for route field definition.

…fix metadata for route field.

contrib/chatops/pack.yaml

contrib/chatops/README.jinja

contrib/chatops/CHANGES.md

… the overall change.

…t_notify

nzlosh · 2020-11-13T13:07:24Z

Anything else to get this merged to master?

nmaludy · 2020-11-13T13:25:47Z

@nzlosh can you force push to this branch to kick off e2e tests again please?

arm4b

I left a few comments to address before we can merge the PR.

I have no problem with adding the errbot rule, however the PR does too much and goes further in a way that might be controversial. I also felt like this PR highlights the names and the URLs more, rather then adding code.

Can we focus on the actual rule instead?

CHANGELOG.rst

contrib/chatops/README.md

arm4b · 2020-11-14T14:09:07Z

contrib/chatops/README.md

+> ChatOps integration pack
+StackStorm, Inc. <info@stackstorm.com>
+
+### Contributors


Having the Contributors section as a first thing under the Readme is not an appropriate place.
We also don't list "Contributors" in the Readme for the core stackstorm packs so it's better to remove it.

https://github.com/StackStorm/st2/blob/master/OWNERS.md highlights this instead.

Making a change to the chatops pack, it seems like a bold statement to claim to be a coder owner of st2. Perhaps each pack should have it's own OWNERS.md file to clarify this?

https://github.com/StackStorm/st2/blob/master/OWNERS.md highlights the areas of expertise and responsibilities for the Maintainers and Contributors around StackStorm codebase in general, if it's Chatops, CI, Orquestra or any other systems.

https://docs.github.com/en/free-pro-team@latest/github/creating-cloning-and-archiving-repositories/about-code-owners#example-of-a-codeowners-file is used as well for the projects.

contrib/chatops/README.md

contrib/chatops/rules/notify_errbot.yaml

… reflect the overall change." This reverts commit 256e9d1.

… perms, fix metadata for route field." This reverts commit 9c42e6a.

…gelog to reflect the overall change."" This reverts commit 856393e.

arm4b

Thanks a lot for the changes 👍

We'll merge it today.

arm4b · 2020-11-16T23:35:08Z

StackStorm e2e tests are failing for real in this PR. st2 e2e tests failure:

ok 1 StackStorm's client connection
ok 2 npm directory exists
ok 3 some StackStorm aliases are enabled
not ok 4 chatops.notify rule exists
# (from function `assert_output' in file chatops/../test_helpers/bats-assert/src/assert.bash, line 239,
#  in test file chatops/test_hubot.bats, line 36)
#   `assert_output \"chatops.notify\"' failed
# 
# -- output differs --
# expected (1 lines):
#   chatops.notify
# actual (2 lines):
#   chatops.notify
#   chatops.notify-errbot
# --
# 
ok 5 hubot help command works
ok 6 chatops.post_message execution and receive status works
ok 7 complete request-response flow"

Here is the actual test case: https://github.com/StackStorm/st2tests/blob/6f384e7a03ac55d6b57d3036812b2fd01660ac3e/chatops/test_hubot.bats#L31-L42

@nzlosh Looks like more work needs to be done in st2tests repository as well. Can you provide a PR there?

nzlosh · 2020-11-16T23:54:53Z

I've updated the st2test repo to test for the presence of the notify-errbot rule in the chatops pack.

blag · 2020-11-17T01:02:23Z

The contrib/chatops/rules/notify_hubot.yaml file was chmodded: 100755 → 100644 - was this intentional or can we undo that?

blag · 2020-11-17T01:12:14Z

Tests should be partially fixed when we merge in StackStorm/st2tests#194.

Once that is merged and these tests are passing, we can merge this in.

Once this is merged in, we can merge in StackStorm/st2tests#195 to check for the additional chatops notification channel rule.

arm4b · 2020-11-17T19:28:30Z

Hm, after re-running e2e tests after StackStorm/st2tests#194 they still fail with the same error:

ok 1 StackStorm's client connection
ok 2 npm directory exists
ok 3 some StackStorm aliases are enabled
not ok 4 chatops.notify rule exists
# (from function `assert_output' in file chatops/../test_helpers/bats-assert/src/assert.bash, line 239,
#  in test file chatops/test_hubot.bats, line 41)
#   `assert_output \"true\"' failed
# 
# -- output differs --
# expected (1 lines):
#   true
# actual (2 lines):
#   true
#   true
# --
# 
ok 5 hubot help command works
ok 6 chatops.post_message execution and receive status works
ok 7 complete request-response flow"

From what I understand the tests for this PR are cloned from the master branch of https://github.com/StackStorm/st2tests/
@blag any ideas?

nzlosh · 2020-11-17T20:15:19Z

@blag contrib/chatops/rules/notify_hubot.yaml file was chmodded: 100755 → 100644, yes that was intentional on my part, yaml is not an executable file, so I corrected it.

blag · 2020-11-18T11:06:12Z

EL7 end-to-end tests are now failing due to Python 2/3 issues:

not ok 36 packs.setup_virtualenv with python3 flag works
# (in test file cli/test_pack_python3.bats, line 88)
#   `RESULT=$(st2 run examples.python_runner_print_python_version -j)' failed
not ok 37 python3 imports work correctly
# (from function `assert_success' in file cli/../test_helpers/bats-assert/src/assert.bash, line 114,
#  in test file cli/test_pack_python3.bats, line 117)
#   `assert_success' failed
# 
# -- command failed --
# status : 1
# output (19 lines):
#   
#   {
#       \"action\": {
#           \"ref\": \"python3_test.test_stdlib_import\"
#       }, 
#       \"context\": {
#           \"user\": \"st2admin\"
#       }, 
#       \"end_timestamp\": \"2020-11-18T04:43:06.745006Z\", 
#       \"id\": \"5fb4a65a4c2a7dcce5963bfe\", 
#       \"result\": {
#           \"exit_code\": 1, 
#           \"result\": \"None\", 
#           \"stderr\": \"Traceback (most recent call last):\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/netaddr/compat.py\\\", line 91, in <module>\
    from importlib import resources as _importlib_resources\
ImportError: cannot import name 'resources'\
\
During handling of the above exception, another exception occurred:\
\
Traceback (most recent call last):\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/python_runner/python_action_wrapper.py\\\", line 53, in <module>\
    from st2common import log as logging\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/st2common/log.py\\\", line 32, in <module>\
    from st2common.logging.handlers import FormatNamedFileHandler\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/st2common/logging/handlers.py\\\", line 22, in <module>\
    from oslo_config import cfg\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/oslo_config/cfg.py\\\", line 336, in <module>\
    from oslo_config import types\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/oslo_config/types.py\\\", line 21, in <module>\
    import netaddr\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/netaddr/__init__.py\\\", line 18, in <module>\
    from netaddr.core import (AddrConversionError, AddrFormatError,\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/netaddr/core.py\\\", line 11, in <module>\
    from netaddr.compat import _callable, _iter_dict_keys\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/netaddr/compat.py\\\", line 93, in <module>\
    import importlib_resources as _importlib_resources\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/importlib_resources/__init__.py\\\", line 5, in <module>\
    from ._common import (\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/importlib_resources/_common.py\\\", line 9, in <module>\
    from ._compat import (\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/importlib_resources/_compat.py\\\", line 52, in <module>\
    from typing import runtime_checkable  # type: ignore\
  File \\\"/opt/stackstorm/st2/lib/python2.7/site-packages/typing.py\\\", line 782, in <module>\
    AnyStr = TypeVar('AnyStr', bytes, unicode)\
NameError: name 'unicode' is not defined\
\", 
#           \"stdout\": \"\"
#       }, 
#       \"start_timestamp\": \"2020-11-18T04:43:06.273452Z\", 
#       \"status\": \"failed\"
#   }
# --
#

arm4b · 2020-11-21T15:06:15Z

I think the e2e tests should be fixed at this point.
@nzlosh please update the Changelog to fix the git conflict. That'll re-trigger the CI as well.

nzlosh added 2 commits October 2, 2020 13:32

Add notify-errbot rule, update readme using pack2md, fix file perms, …

9c42e6a

…fix metadata for route field.

Merge branch 'master' into errbot_notify

9112992

nmaludy reviewed Oct 2, 2020

View reviewed changes

contrib/chatops/pack.yaml Show resolved Hide resolved

contrib/chatops/README.jinja Outdated Show resolved Hide resolved

contrib/chatops/CHANGES.md Show resolved Hide resolved

nzlosh added 2 commits October 2, 2020 14:40

Increased chatops minor version and updated main changelog to reflect…

256e9d1

… the overall change.

Merge branch 'master' of https://github.com/StackStorm/st2 into errbo…

540e6af

…t_notify

nmaludy approved these changes Nov 13, 2020

View reviewed changes

nmaludy added this to the 3.4.0 milestone Nov 13, 2020

Merge branch 'master' into errbot_notify

4be0940

nzlosh force-pushed the errbot_notify branch from b23ca4e to 4be0940 Compare November 13, 2020 13:31

arm4b suggested changes Nov 16, 2020

View reviewed changes

nzlosh added 3 commits November 16, 2020 16:52

Revert "Increased chatops minor version and updated main changelog to…

856393e

… reflect the overall change." This reverts commit 256e9d1.

Revert "Add notify-errbot rule, update readme using pack2md, fix file…

b64cc02

… perms, fix metadata for route field." This reverts commit 9c42e6a.

Revert "Revert "Increased chatops minor version and updated main chan…

94cf021

…gelog to reflect the overall change."" This reverts commit 856393e.

pull-request-size bot added the size/XS PR that changes 0-9 lines. Quick fix/merge. label Nov 16, 2020

Clean up for PR.

f28ed66

arm4b approved these changes Nov 16, 2020

View reviewed changes

arm4b added the chatops label Nov 16, 2020

Merge branch 'master' into errbot_notify

94b31d4

nzlosh mentioned this pull request Nov 16, 2020

Test for notify-errbot, since rule is added to chatops pack. StackStorm/st2tests#193

Closed

This was referenced Nov 17, 2020

Allow other notification channels besides hubot StackStorm/st2tests#194

Merged

Allow errbot notify rule StackStorm/st2tests#195

Merged

nzlosh added 2 commits November 21, 2020 18:13

Merge branch 'master' into errbot_notify

87b7f18

Merge branch 'errbot_notify' of github.com:nzlosh/st2 into errbot_notify

db340e8

arm4b merged commit 963f97f into StackStorm:master Nov 21, 2020

nzlosh deleted the errbot_notify branch November 21, 2020 19:35

Uh oh!

Conversation

nzlosh commented Oct 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nzlosh commented Nov 13, 2020

Uh oh!

nmaludy commented Nov 13, 2020

Uh oh!

arm4b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arm4b Nov 14, 2020

Choose a reason for hiding this comment

Uh oh!

nzlosh Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

arm4b Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arm4b left a comment

Choose a reason for hiding this comment

Uh oh!

arm4b commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nzlosh commented Nov 16, 2020

Uh oh!

blag commented Nov 17, 2020

Uh oh!

blag commented Nov 17, 2020

Uh oh!

arm4b commented Nov 17, 2020

Uh oh!

nzlosh commented Nov 17, 2020

Uh oh!

blag commented Nov 18, 2020

Uh oh!

arm4b commented Nov 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arm4b commented Nov 16, 2020 •

edited

Loading