poll-daemon

Merge lp://staging/~nikwen/account-polld/more-flexible-account-authentication into lp://staging/~ubuntu-push-hackers/account-polld/trunk

  1. more-flexible-account-authentication
  2. Merge into trunk
Proposed by Niklas Wenzel
Status: Needs review
Proposed branch: lp://staging/~nikwen/account-polld/more-flexible-account-authentication
Merge into: lp://staging/~ubuntu-push-hackers/account-polld/trunk
Diff against target: 356 lines (+133/-59)
7 files modified
accounts/account-watcher.c (+81/-24)
accounts/account-watcher.h (+4/-4)
accounts/accounts.c (+3/-4)
accounts/accounts.go (+26/-17)
cmd/account-polld/main.go (+1/-1)
plugins/gmail/gmail.go (+7/-3)
plugins/twitter/twitter.go (+11/-6)
To merge this branch: bzr merge lp://staging/~nikwen/account-polld/more-flexible-account-authentication
Reviewer Review Type Date Requested Status
Alberto Mardegan (community) Needs Fixing
Ubuntu Push Hackers Pending
Review via email: mp+287417@code.staging.launchpad.net

Commit message

Make account authentication more flexible, easing the addition of new authentication methods in the future, and add password authentication

Description of the change

Make account authentication more flexible, easing the addition of new authentication methods in the future, and add password authentication

Required to add SASL authentication for IMAP support

To post a comment you must log in.
Revision history for this message
Alberto Mardegan (mardy) wrote :

Hi Niklas, this is very good work, thanks! :-)

I've written a few inline comments, please have a look at them. As a general remark, I can say that the code looks very good (though I wonder, is there a better way to pass the Data to the Go module, without having the two arrays of keys and values being passed separately? Is it possible to construct the Go dictionary in C?), but I believe that you haven't tested it with Twitter, because -- just by looking at the code, so I might be wrong -- it seems to me that the changes must have broken it.

Once you fix the various issues, could you please also test the Twitter integration and make sure that it continues working?

review: Needs Fixing
Reply
Revision history for this message
Niklas Wenzel (nikwen) wrote :

Hey Alberto,

Thanks a lot for your review! :)

You were right that I didn't test the Twitter plugin. As I do not have a Twitter account, I had never really looked too deeply into the Twitter code and straight out assumed that it used OAuth 2.0. I'll fix that.

Thanks again! :)

Reply

Unmerged revisions

155. By Niklas Wenzel

Fix build errors

154. By Niklas Wenzel

Improve error messages

153. By Niklas Wenzel

Formatting fix

152. By Niklas Wenzel

Make account authentication more flexible, easing the addition of new auth methods

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches