-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add st2chatops role #125
Add st2chatops role #125
Conversation
Installation looks pretty clean. From the configuration point of view we'll need the following syntax to work: st2chatops_version: latest
# hash to configure any value in st2chatops.env
# https://github.com/StackStorm/st2chatops/blob/master/st2chatops.env
st2chatops_config:
ST2_API_KEY: 123asd456
HUBOT_ADAPTER: slack
HUBOT_SLACK_TOKEN: xoxb-CHANGE-ME-PLEASE
... |
Awesome!! That was my next question, whether we need configuration or not. Thanks!! |
roles/st2chatops/tasks/main.yml
Outdated
state: present | ||
when: st2chatops_version != "latest" | ||
notify: | ||
- restart mistral |
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.
Copy pasta
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.
Haha, right in the bull's-eye! 🎯 🥇
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.
haha, WIP folks 😉
README.md
Outdated
| `st2mistral_db` | `mistral` | PostgreSQL DB name for Mistral. | ||
| `st2mistral_db_username` | `mistral` | PostgreSQL DB user for Mistral. | ||
| `st2mistral_db_password` | `StackStorm` | PostgreSQL DB password for Mistral. | ||
| **st2chatops** | Default hubot adapter: 'slack' | ||
| `st2chatops_version` | `latest` | st2mistral version to install. Use `latest` to get automatic updates or pin it to numeric version like `2.2.0`. | ||
| `st2chatops_ST2_API_KEY` | `CHANGE-ME-PLEASE` | st2 API key, that can be generated using `st2 apikey create -k` in a task. |
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.
The problem with this approach is that we need a new var for every setting in st2chatops.env
. And if we introduce new setting in st2chatops (like recent Mattermost support https://github.com/StackStorm/st2chatops/pull/65/files) it will force us to edit ansible-st2
as well.
The better approach is to have st2chatops_config
hash/dict which will allow us to configure ANY env variable in st2chatops.env
.
Take a look at my comment, we need the following syntax to work: #125 (comment)
# hash to configure any value in st2chatops.env
# https://github.com/StackStorm/st2chatops/blob/master/st2chatops.env
st2chatops_config:
ST2_API_KEY: 123asd456
HUBOT_ADAPTER: slack
HUBOT_SLACK_TOKEN: xoxb-CHANGE-ME-PLEASE
# add here any other env var to configure
At a high level we just have {{ st2chatops_config }}
var to rule all config settings.
Similar approach is implemented in https://github.com/StackStorm/ansible-st2/pull/121/files
roles/st2chatops/tasks/main.yml
Outdated
become: yes | ||
replace: | ||
dest: /opt/stackstorm/chatops/st2chatops.env | ||
regexp: '^# (export HUBOT_SLACK_TOKEN.).*' |
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.
This is still not scalable and handling every single env var is error-prone.
We need better approach.
When {{ st2chatops_config }}
"dict" is provided, - just iterate over key/value pairs and set those in st2chatops.env
.
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.
So with {{ st2chatops_config }}
user could midify any existing env variable from the https://github.com/StackStorm/st2chatops/blob/master/st2chatops.env.
This should work (#commented
vars):
st2chatops_config:
HUBOT_ADAPTER: irc
HUBOT_IRC_SERVER: example-server.com
HUBOT_IRC_PORT: 100500
With that, keep in mind to cover corner cases like handling both "#commented
" and "uncommented" records in st2chatops.env
. Eg. if user provided HUBOT_ADAPTER: IRC
in {{ st2chatops_config }}
, - we should uncomment that var and set value. If record is already uncommented, - just set new val.
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.
@humblearner Another corner case.
If user provided completely new env var like FOO: bar
via {{ st2chatops_config }}
, - should we write that key:value in st2chatops.env
too or maybe throw an error that such env var is not available. WDYT?
(new vars):
st2chatops_config:
FOO: bar
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'd rather not go through all of this and have a .j2 file for each adapter in files/ with jinja templates for the settings. So based on the adapter variable you can generate a config from ansible with variables substituted and placed in the st2chatops.env file on the box.
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.
The problem with that approach is described here: #125 (comment)
Ex: @emedvedev merges Mattermost support (https://github.com/StackStorm/st2chatops/pull/65/files) and we obviously won't include env vars in ansible-st2
.
Another example is that the list of vars we list in st2chatops.env
is not full and can't be. There are a bunch of custom env vars for this or that adapter and even Hubot itself.
Just a quick example from search:
- https://github.com/github/hubot/blob/498d65611388d2881e6ae24f74bd68a832a24211/src/robot.coffee#L413-L417
- https://github.com/github/hubot/blob/498d65611388d2881e6ae24f74bd68a832a24211/bin/hubot#L24-L30
Who knows about more vars hidden in the code?
I don't even want to care about it, it's user's problem. They know what they need, - let's give an instrument.
BTW this approach is similar to #121 to avoid relying on
st2.conf
templating which can become outdated quickly.
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 don't even want to care about it, it's user's problem.
No. We explicitly decided that we'll supply this in commented form in our config file so they don't have to go guess. If someone is adding support for an adapter here, they should include a .j2 file. A .j2 file is a schema for the adapter configuration. You won't have the following problem if you went with this approach.
should we write that key:value in st2chatops.env too or maybe throw an error that such env var is not available. WDYT?
Who knows about more vars hidden in the code?
Exactly the problem we should solve and not leave it to user
we obviously won't include env vars in ansible-st2.
I'd validate the "adapter" variable against a set of supported adapters. A supported adapter means presence of .j2 file.
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.
Exactly the problem we should solve and not leave it to user
I just demonstrated with links how we'd fail to do that.
We can recommend to set some frequently used vars in st2chatops.env but we literally can't inspect every new hubot adapter for some hidden vars. And even by adding new vars you can't monitor all those adapters when new env var is added or new feature is implemented.
Additionally, in st2chatops.env user can add any new env var he wants. Think about custom hubot plugins enabled which is usually the case for real installations. So env vars in reality are used not just to configure predefined list of adaptors, but much more.
By hardcoding I think we bring more work for us and less flexibility for users.
If someone is adding support for an adapter here
None will be adding support for adapter here after adding it in st2chatops or hubot-stackstorm. I had enough cases to be realistic about this when different repos become unsync quickly.
That's the reason I'm implementing config setting in a "free form" #121 rather then hardcoded key-vals for every setting. And I would like us to be consistent in this approach across entire Ansible repository to keep the configuration repo management pragmatic & balanced.
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.
Oh you two. What a silly little tussle. It's adorable to be honest.
I'm inclined to agree w/ @armab but would like to see how it would get implemented. I know how I would do it, but would like to really see what you're thinking. Seems like the "harder" of the two to implement but one that will cause least amount of work in the future.
On the flip side I wonder if @armab is worried about stuff that we just flat out aren't going to run into often and the type of person that runs into it will probably have the ability to fix it anyway.
@armab can you or @humblearner contribute the fix for this quickly? If not I'd say just let @humblearner implement it with J2.
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.
Just googled a bit more about other implementations and "what could be expected".
See these config examples, looks pretty reasonable:
Ansible example for Hubot: https://github.com/brianshumate/ansible-hubot
Chef example for Hubot: https://github.com/chef-cookbooks/hubot#attributes
I think we can do it the same way:
Make {{ st2chatops_adapter }}
var required (since hubot won't work without adapter) and additional env vars via {{ st2chatops_config }}
dict optional.
The problem with hardcoded vars is that apart of "adapter vars" there are also hubot plugin vars:
https://github.com/brianshumate/ansible-hubot/blob/5a1d679472ebfd12119cc9db0209a219560cc6ab/defaults/main.yml#L61
roles/st2chatops/defaults/main.yml
Outdated
# https://github.com/StackStorm/st2chatops/blob/master/st2chatops.env | ||
st2chatops_config: | ||
ST2_API_KEY: CHANGE-ME-PLEASE | ||
HUBOT_ADAPTER: slack |
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 think we shouldn't set HUBOT_ADAPTER: slack
by default if user didn't provide any custom key: value
for that.
WDYT?
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.
These are supposed to contain sane and safe defaults. I am fine with this TBH.
And yeah, we want to include With that, we somehow need to find the way to disable debug messages for specific tasks to avoid showing token in Travis build logs: https://travis-ci.org/StackStorm/ansible-st2/jobs/209102385 |
Just a note: As a good configuration management citizens we'll need ansible vars to install & enable hubot external scripts/plugins from http://hubot-script-catalog.herokuapp.com/ Same as it's implemented in https://github.com/brianshumate/ansible-hubot and https://github.com/chef-cookbooks/hubot#attributes Not a requirement for this PR, - can wait for future. |
Alright!! After long discussion and tie-breaker (Thanks @bigmstone!!). We are going with For now, I have kept the Configure adapter setting task in FYI.. going with ToDo:
|
@humblearner Yeah, that sounds good enough. I thought it will show real API_KEY in the log. Vault is not necessarry, since we don't want our keys (even encrypted) to be distributed with the repo. But as additional precautionary measure adding |
I'm not sure if we need such validation, since with There are a bunch of variables you can set, depending on Hubot plugins installed: export FILE_BRAIN_PATH
export HUBOT_SHIP_EXTRA_SQUIRRELS
export HUBOT_ANNOUNCE_ROOMS and so we can't and don't want to control that via any kind of rules. |
roles/st2chatops/meta/main.yml
Outdated
- st2 | ||
- devops | ||
- chatops | ||
- autmation |
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.
automation
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.
Also i think we can include additional hubot
tag.
roles/st2chatops/meta/main.yml
Outdated
- chatops | ||
- autmation | ||
dependencies: | ||
- role: st2 |
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.
st2
role is not a requirement for st2chatops
since st2chatops
can be installed on a separated machine, it's possible to configure remote API endpoints and it will work.
Saying that, st2repo
is a dependency for sure to install st2chatops package.
cc @lakshmi-kannan you asked before why we need st2repo
and st2
roles separated.
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.
Apparently, it should. However, it fails here:
RUNNING HANDLER [st2chatops : restart st2chatops] ******************************
fatal: [centos6]: FAILED! => {"changed": false, "failed": true, "msg": "touch: cannot touch `/var/log/st2/st2chatops.log': No such file or directory\nrunuser: group st2 does not exist\n"}
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.
Nice! This sounds like a bug discovered.
st2chatops can be configured on separated machine (https://github.com/StackStorm/st2chatops/blob/master/st2chatops.env#L23) and should not rely on resources created by st2 package.
Can you document the bug in st2chatops repo? It's a packaging issue, we can fix it later.
roles/st2chatops/defaults/main.yml
Outdated
st2chatops_ST2_API_KEY: "CHANGE-ME-PLEASEdd" | ||
|
||
# Hubot Adapter to be used for st2chatops | ||
st2chatops_HUBOT_ADAPTER: CHANGE-ME-PLEASE |
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.
Just an empty string or None (which won't set the val in st2chatops.env
) as a default would be nice.
Also I'm more in favor of lowercase vars like: st2chatops_adapter
or if you want st2chatops_hubot_adapter
.
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.
roles/st2chatops/defaults/main.yml
Outdated
st2chatops_version: latest | ||
|
||
# Please provide ST2_API_KEY using "st2 apikey create -k" | ||
st2chatops_ST2_API_KEY: "CHANGE-ME-PLEASEdd" |
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.
Same as previous comment, just an empty string or maybe None
(which won't set the val in st2chatops.env) instead of CHANGE-ME-PLEASEdd
will look nicer.
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.
CHANGE-ME-PLEASE
is similar to https://github.com/brianshumate/ansible-hubot/blob/master/defaults/main.yml#L58. Better explicit than implicit. 🙂
roles/st2chatops/meta/main.yml
Outdated
description: Install st2chatops | ||
author: humblearner | ||
company: StackStorm | ||
license: Apache |
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.
Apache 2.0
roles/st2chatops/tasks/main.yml
Outdated
- '^(export ST2_AUTH_USERNAME.*)' | ||
- '^(export ST2_AUTH_PASSWORD.*)' | ||
when: st2chatops_api_key.changed | ||
tags: [st2chatops, skip_ansible_lint] |
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.
Same as before, any touch of st2chatops.env
should trigger st2chatops restart
handler.
Make sure we follow that in all other Ansible tasks.
roles/st2chatops/vars/main.yml
Outdated
@@ -0,0 +1,9 @@ | |||
--- | |||
supported_hubot_adapter: |
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.
supported_hubot_adapters
roles/st2chatops/tasks/main.yml
Outdated
dest: /opt/stackstorm/chatops/st2chatops.env | ||
regexp: '^# (export HUBOT_ADAPTER={{ st2chatops_HUBOT_ADAPTER }})$' | ||
replace: '\1' | ||
when: st2chatops_api_key.changed and st2chatops_HUBOT_ADAPTER in supported_hubot_adapter |
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.
st2chatops_HUBOT_ADAPTER in supported_hubot_adapter
In this case it will skip this important task if adapater is wrong.
I think it makes sense to move this validation logic in separated task and FAIL playbook execution early and loud if user provided invalid adapter.
Ex how @lakshmi-kannan did it:
ansible-st2/roles/bwc_repos/tasks/bwc_repos_apt.yml
Lines 3 to 5 in 1aa9d35
- name: Assert that master_token is specified | |
fail: msg="License key must be supplied for BWC enterprise installation." | |
when: license is not defined |
roles/st2chatops/tasks/main.yml
Outdated
with_dict: "{{ st2chatops_config }}" | ||
loop_control: | ||
loop_var: _conf_option | ||
when: uncomment_adapter.changed # TODO: Add check for Individual keys in hash |
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.
This will fail to work in cases when user needs to re-configure adapter settings.
Ex: change API key, but didn't change adapter.
Consider this Ansible role not as a way to install st2chatops
role only once, but in a wide conext of management where different values could be changed/reconfigured with time.
README.md
Outdated
| `st2mistral_db` | `mistral` | PostgreSQL DB name for Mistral. | ||
| `st2mistral_db_username` | `mistral` | PostgreSQL DB user for Mistral. | ||
| `st2mistral_db_password` | `StackStorm` | PostgreSQL DB password for Mistral. | ||
| **st2chatops** | ||
| `st2chatops_config` | `{ }` | Hash of st2chatops settings to edit in [`st2chatops.env`](https://github.com/StackStorm/st2chatops/blob/master/st2chatops.env) file. For example, `Slack` hubot adapter requires: `ST2_API_KEY: CHANGE-ME-PLEASE` `HUBOT_ADAPTER: slack` `HUBOT_SLACK_TOKEN: xoxb-CHANGE-ME-PLEASE` |
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.
With 2 new jinja vars, seems ST2_API_KEY
and HUBOT_ADAPTER
are irrelevant in this description now.
We just need them in README.
Curious how We need to add some simple check in |
- flowdock | ||
- yammer | ||
- spark | ||
- irc |
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.
Please also include shell
adapter: https://github.com/github/hubot/blob/master/docs/adapters/shell.md
BTW do you think it will be better if we use shell
adapter instead of Slack for testing/smoketests purposes in TravisCI?
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.
Also we need mattermost
adapter.
Just to be one step ahead because of this addition: StackStorm/st2chatops#65 I'm sure we'll forget to add it later.
When all finished/merged, we'll need Ansible |
is why i had ToDo task:
However, I am perfectly fine, leaving it open for the user to decide, what they want to add to their hubot adapter settings. |
Yeah, I think that corner case is irrelevant now, considering research and fact that env variable could be anything from the http://hubot-script-catalog.herokuapp.com/, not just adapter-specific. |
roles/st2chatops/tasks/main.yml
Outdated
tags: st2chatops | ||
|
||
|
||
- name: Uncomment Hubot "{{ st2chatops_hubot_adapter|upper }}" adapter in "/opt/stackstorm/chatops/st2chatops.env" |
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.
What if i specify st2chatops_hubot_adapter=slack
in Ansible and then change it to st2chatops_hubot_adapter=hipchat
or any other adapter later?
Will it work?
roles/st2chatops/tasks/main.yml
Outdated
- name: Assert st2chatops_hubot_adapter is specified | ||
fail: | ||
msg: > | ||
'"st2chatops_hubot_adapter" must be supplied in roles/st2chatops/defaults/main.yml for st2chatops installation. |
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.
We can omit mentioning roles/st2chatops/defaults/main.yml
since it's not single point of truth.
Just asking user to provide {{ st2chatops_hubot_adapter }}
var is enough.
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 guess, you mean https://github.com/StackStorm/ansible-st2/blob/master/roles/st2chatops/vars/main.yml
Sync-up with Additionally, as said before, we'll need some smoke test case to make sure |
- yammer | ||
- spark | ||
- irc | ||
- mattermost |
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.
Just discovered in st2chatops
PR that the right naming for mattermost adapter is matteruser
, see: https://github.com/StackStorm/st2chatops/blob/1164c0823885931e3a3ad48e0cbb52a336f2d01b/st2chatops.env#L107
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.
FWIW, mattermost
adapter should work as well, and we have support for it in hubot-stackstorm, but @armab is right, matteruser
is the one we pack in st2chatops
.
roles/st2smoketests/tasks/main.yml
Outdated
replace: '{{ st2chatops_api_key }}' | ||
no_log: true | ||
changed_when: no | ||
notify: restart st2chatops |
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.
This is a catch-22 situation. Either restart or idempotency. Removing this and https://github.com/StackStorm/ansible-st2/pull/125/files#diff-c6fa708e733eecbe8e8a283eb5690c19R124 in favor of: https://github.com/StackStorm/ansible-st2/pull/125/files#diff-c6fa708e733eecbe8e8a283eb5690c19R140
roles/st2smoketests/tasks/main.yml
Outdated
- smoke-tests | ||
|
||
- name: Verify st2chatops using bin/hubot | ||
shell: "cd /opt/stackstorm/chatops/ && { echo -n; sleep 5; echo '! st2 list actions'; echo; sleep 10; } | bin/hubot --test " |
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.
With bin/hubot --test
we actually test shell
adapter.
See: https://github.com/StackStorm/st2chatops/blob/master/bin/hubot#L20-L22
From the lines above I see you're setting HUBOT_SLACK_TOKEN
So decide whether you test slack
or shell
hubot adapter.
roles/st2chatops/tasks/main.yml
Outdated
- name: Assert st2chatops_hubot_adapter is specified | ||
fail: | ||
msg: > | ||
'"st2chatops_hubot_adapter" must be supplied in roles/st2chatops/defaults/main.yml for st2chatops installation.' |
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.
This error message looks not exactly what we check, according to this conditional:
when: st2chatops_hubot_adapter not in supported_hubot_adapters
As I understand we really check if {{ st2chatops_hubot_adapter }}
is supported, but instead of that throwing an error saying that {{ st2chatops_hubot_adapter }}
is not provided in roles/st2chatops/defaults/main.yml
.
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.
Looks nice/works well 👍
Just a final request, - can you please also extract chatops smoketests into separated file st2chatops.yml
under the st2sometests
role?
It will be easier to add when
conditional in future, ex: run st2chatops checks only when respective package is installed.
Just FYI, once merged I'll see what I can do to polish corner cases similar to that a bit later.
Once merged what we'll need also:
|
Sample for st2chatops role
For st2_api_key generation task: https://github.com/StackStorm/ansible-st2/pull/125/files#diff-c6fa708e733eecbe8e8a283eb5690c19R53