-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Conversation
Interesting approach moving that out to a shared module! I think that's a first for a hubot-scripts thing. Perhaps instead of breaking things by changing the expected variable, could still use the HUBOT_TWITTER_ACCESS_TOKEN_KEY and HUBOT_TWITTER_ACCESS_TOKEN_SECRET, where those would be the 'default credentials'. |
access_token_secret: process.env.HUBOT_TWITTER_ACCESS_TOKEN_SECRET | ||
auth = twitterConfig.defaultCredentials() | ||
unless auth | ||
console.log "Please set HUBOT_TWITTER_CONSUMER_KEY_<USERNAME> and HUBOT_TWITTER_CONSUMER_SECRET_<USERNAME>." |
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 would move this into the robot.respond
block, and make it do msg.send
. For users of the system, the command would just fail, and it'd take some digging into the logs to find the output for this little console.log
.
It might make sense to add a helper to twitterConfig that takes a msg
object so it can output what goes wrong. Like:
auth = twitterConfig.defaultCredentials()
unless auth.checkEnvironment(msg)
return
Then have checkEnvironment do the msg.send
for errors, and return false if something isn't set.
HUBOT_TWITTER_ACCESS_TOKEN_SECRET, if specified
Ok, I think all has been addressed, minus the |
I want to try this out locally, but really like the direction this is going! |
I'm pretty sure we'd need separate |
@afeld yeah, this would need to track consumer key & secret per twitter account in addition to the access key & secret. |
@@ -8,8 +8,8 @@ | |||
# Configuration: | |||
# HUBOT_TWITTER_CONSUMER_KEY | |||
# HUBOT_TWITTER_CONSUMER_SECRET | |||
# HUBOT_TWITTER_ACCESS_TOKEN_KEY | |||
# HUBOT_TWITTER_ACCESS_TOKEN_SECRET | |||
# HUBOT_TWITTER_ACCESS_TOKEN_KEY_<USERNAME> |
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 can remove the _ from this one. In this particular script, we wouldn't support multiple accounts.
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.
My thinking was that HUBOT_TWITTER_ACCESS_TOKEN_KEY
and HUBOT_TWITTER_ACCESS_TOKEN_SECRET
are deprecated - no need in having two environment variables for the same value, other than to explicitly specify the default one (though it really doesn't matter which account it uses).
Updated to gracefully degrade when environment variables aren't set. |
Conflicts: package.json
We are actually moving away from adding scripts to repository in favor of separate npm packages per scripts. See #1113 for details, and let us know if you have any questions about getting going with that! |
This also changes (and normalizes) the way that the Twitter keys/secrets are specified, which is a breaking change.
/cc @technicalpickles @jhubert