-
-
Notifications
You must be signed in to change notification settings - Fork 782
Add notify-errbot rule to ChatOps pack. #5051
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
Conversation
…fix metadata for route field.
|
Anything else to get this merged to master? |
|
@nzlosh can you force push to this branch to kick off e2e tests again please? |
b23ca4e to
4be0940
Compare
arm4b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
contrib/chatops/README.md
Outdated
| > ChatOps integration pack | ||
| StackStorm, Inc. <info@stackstorm.com> | ||
| ### Contributors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
arm4b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the changes 👍
We'll merge it today.
|
StackStorm e2e tests are failing for real in this PR. st2 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 |
|
I've updated the st2test repo to test for the presence of the notify-errbot rule in the chatops pack. |
|
The |
|
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. |
|
Hm, after re-running e2e tests after StackStorm/st2tests#194 they still fail with the same error: From what I understand the tests for this PR are cloned from the master branch of https://github.com/StackStorm/st2tests/ |
|
@blag |
|
EL7 end-to-end tests are now failing due to Python 2/3 issues: |
|
I think the e2e tests should be fixed at this point. |
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.