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

(WIP) Remove undocumented ST2_AUTH_TOKEN from st2chatops #181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jinpingh
Copy link
Contributor

ST2_AUTH_TOKEN is not documented in st2chatops.env and because the token either has a long expiry time or the expiry time is so short the ChatOps service is restarted all the time to pick up the config changes. It is no point to keep it, therefore remove this auth method from hubot-stackstorm entirely.

`ST2_AUTH_TOKEN` is not documented in `st2chatops.env` and because the token either has a long expiry time or the expiry time is so short the ChatOps service is restarted all the time to pick up the config changes. It is no point to keep it, therefore remove this auth method from hubot-stackstorm entirely.
@arm4b
Copy link
Member

arm4b commented Jun 27, 2019

I don't see any significant benefit in removing token comparing to other tasks, but not opposted with the change itself.

To make it happen, please check other StackStorm repos if we use any ST2_AUTH_TOKEN with chatops, for example in tests, CI, Docker images or other possible places.

Additionally, don't forget about changelog record and package version change.

@jinpingh
Copy link
Contributor Author

To make it happen, please check other StackStorm repos if we use any ST2_AUTH_TOKEN with chatops, for example in tests, CI, Docker images or other possible places.

Thanks for point it out and you had good point. Already found some test codes depend on ST2_AUTH_TOKEN.
I am studying coffeescript and read #133 how hubot-stackstorm is refracted. It came ST2_AUTH_TOKEN discussion and don't want too many changes in one activity. I will leave this PR alone for now.

@arm4b
Copy link
Member

arm4b commented Jun 27, 2019

I found this PR pretty minimal and easy to change, I'm probably missing some background discussion around #133.

As for other dependent repos that rely on token in chatops, - it would be nice to see those PRs first.
It looks like we have a freedom of removing all the uses of ST2_AUTH_TOKEN as a step before merging this particular change.

@jinpingh
Copy link
Contributor Author

I found this PR pretty minimal and easy to change, I'm probably missing some background discussion around #133.

As for other dependent repos that rely on token in chatops, - it would be nice to see those PRs first.
It looks like we have a freedom of removing all the uses of ST2_AUTH_TOKEN as a step before merging this particular change.
T

Good point again, thanks!

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants