The Wayback Machine - https://web.archive.org/web/20180604113620/https://github.com/github/hubot-scripts/pull/1179
Skip to content
Learn more
Please note that GitHub no longer supports your web browser.

We recommend upgrading to the latest Google Chrome or Firefox.

This repository has been archived by the owner. It is now read-only.

Centralize Twitter config #1179

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
2 participants
@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

This comment has been minimized.

Show comment Hide comment
@technicalpickles

technicalpickles Oct 10, 2013

Member

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

Member

technicalpickles commented Oct 10, 2013

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

src/scripts/tweet.coffee
- 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>."

This comment has been minimized.

Show comment Hide comment
@technicalpickles

technicalpickles Oct 10, 2013

Member

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.

@technicalpickles

technicalpickles Oct 10, 2013

Member

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.

src/scripts/tweeter.coffee
-# "github" : { "access_token" : "", "access_token_secret" : ""}
-# }
+# See here for environment variable descriptions:
+# https://github.com/github/hubot-scripts/tree/master/src/twitter-config.js

This comment has been minimized.

Show comment Hide comment
@technicalpickles

technicalpickles Oct 10, 2013

Member

Wouldn't this be .coffee?

@technicalpickles

technicalpickles Oct 10, 2013

Member

Wouldn't this be .coffee?

@afeld

This comment has been minimized.

Show comment Hide comment
@afeld

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

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

This comment has been minimized.

Show comment Hide comment
@technicalpickles

technicalpickles Oct 15, 2013

Member

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

Member

technicalpickles commented Oct 15, 2013

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

@technicalpickles

This comment has been minimized.

Show comment Hide comment
@technicalpickles

technicalpickles Oct 15, 2013

Member

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.

Member

technicalpickles commented Oct 15, 2013

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

This comment has been minimized.

Show comment Hide comment
@technicalpickles

technicalpickles Oct 15, 2013

Member

@afeld yeah, this would need to track consumer key & secret per twitter account in addition to the access key & secret.

Member

technicalpickles commented Oct 15, 2013

@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>

This comment has been minimized.

Show comment Hide comment
@technicalpickles

technicalpickles Nov 15, 2013

Member

I think we can remove the _ from this one. In this particular script, we wouldn't support multiple accounts.

@technicalpickles

technicalpickles Nov 15, 2013

Member

I think we can remove the _ from this one. In this particular script, we wouldn't support multiple accounts.

This comment has been minimized.

Show comment Hide comment
@afeld

afeld Nov 15, 2013

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

afeld Nov 15, 2013

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

This comment has been minimized.

Show comment Hide comment
@afeld

afeld Nov 16, 2013

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

afeld commented Nov 16, 2013

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

@technicalpickles

This comment has been minimized.

Show comment Hide comment
@technicalpickles

technicalpickles May 6, 2016

Member

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!

Member

technicalpickles commented May 6, 2016

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.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.
Press h to open a hovercard with more details.
Morty Proxy This is a proxified and sanitized view of the page, visit original site.