Skip to content
This repository has been archived by the owner on Jun 8, 2023. It is now read-only.

Centralize Twitter config #1179

Closed
wants to merge 11 commits into from
Closed

Centralize Twitter config #1179

wants to merge 11 commits into from

Conversation

afeld
Copy link

@afeld afeld commented Oct 10, 2013

This also changes (and normalizes) the way that the Twitter keys/secrets are specified, which is a breaking change.

/cc @technicalpickles @jhubert

@technicalpickles
Copy link
Contributor

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>."
Copy link
Contributor

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.

@afeld
Copy link
Author

afeld commented Oct 11, 2013

Ok, I think all has been addressed, minus the checkEnvironment() thing - I just couldn't think of a super clean way to handle it.

@technicalpickles
Copy link
Contributor

I want to try this out locally, but really like the direction this is going!

@technicalpickles
Copy link
Contributor

I'm pretty sure we'd need separate HUBOT_TWITTER_CONSUMER_KEY and HUBOT_TWITTER_CONSUMER_KEY per account, because each account would have it's own OAuth on dev.twitter.com for accessing things.

@technicalpickles
Copy link
Contributor

@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>
Copy link
Contributor

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.

Copy link
Author

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).

@afeld
Copy link
Author

afeld commented Nov 16, 2013

Updated to gracefully degrade when environment variables aren't set.

@technicalpickles
Copy link
Contributor

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!

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

Successfully merging this pull request may close these issues.

2 participants