poll-daemon

Merge lp://staging/~jamesh/account-polld/account-refresh into lp://staging/~phablet-team/account-polld/trunk

  1. account-refresh
  2. Merge into trunk
Proposed by James Henstridge
Status: Merged
Approved by: Sergio Schvezov
Approved revision: 30
Merged at revision: 21
Proposed branch: lp://staging/~jamesh/account-polld/account-refresh
Merge into: lp://staging/~phablet-team/account-polld/trunk
Diff against target: 868 lines (+306/-123)
18 files modified
accounts/account-watcher.c (+80/-55)
accounts/account-watcher.h (+4/-1)
accounts/accounts.c (+16/-10)
accounts/accounts.go (+47/-14)
cmd/account-polld/account_manager.go (+30/-18)
cmd/account-polld/main.go (+8/-4)
cmd/account-watcher-test/main.go (+6/-2)
data/account-polld.application (+5/-11)
data/account-polld.service-type (+7/-0)
data/facebook-poll.service (+8/-0)
data/google-gmail-poll.service (+1/-1)
data/twitter-poll.service (+8/-0)
debian/rules (+4/-1)
plugins/facebook/facebook.go (+3/-0)
plugins/facebook/facebook_test.go (+25/-5)
plugins/plugins.go (+9/-1)
plugins/twitter/twitter.go (+12/-0)
plugins/twitter/twitter_test.go (+33/-0)
To merge this branch: bzr merge lp://staging/~jamesh/account-polld/account-refresh
Reviewer Review Type Date Requested Status
Sergio Schvezov Approve
PS Jenkins bot continuous-integration Approve
Review via email: mp+227759@code.staging.launchpad.net

Commit message

Add support for handling token expiry.

Description of the change

Add support for handling token expiry. The basic mechanism is that:

1. plugins convert their "expired token" errors into the plugins.ErrTokenExpired error.
2. the AccountManager handles this error by calling watcher.Refresh(accountId)
3. the new token is returned via the watcher's channel, as with other changes.

Along the way, I made a few changes:

* the accounts package now exposes a Watcher object holding the channel. This was so I'd have somewhere to hang the Refresh() method.
* I replaced the AccountManager's terminate channel with a channel for receiving the AuthData. Closing this channel is used as a terminate signal, and removed the need for a mutex.

To post a comment you must log in.
Revision history for this message
Sergio Schvezov (sergiusens) wrote :

Looks good, just added one minor inline comment.

Another non related to this change comment is to ask if it would be ok to change the fprintf's to call go's log.Print.*

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

ideally we should only print from package main, so this is really optional.

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

I don't think it's catching accounts being created while running, so if I create my twitter account during a polld run it never shows up (I do go into the account and enable/disable the toggle)

if I restart polld the, the account is seen. Looking at the code again, I see only one call to ag_manager_list and no callback setup for new accounts. Should that be fixed in this merge?

Rest of the code looks fine.

review: Needs Information
Revision history for this message
Sergio Schvezov (sergiusens) wrote :

Looking again, it should be handled by account_watcher_enabled_event_cb; not sure why I'm not seeing new accounts then... (I was playing with twitter fwiw).

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:22
http://jenkins.qa.ubuntu.com/job/account-polld-ci/2/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-amd64-ci/2
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-armhf-ci/2
        deb: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-armhf-ci/2/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-i386-ci/2

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/account-polld-ci/2/rebuild

review: Approve (continuous-integration)
lp://staging/~jamesh/account-polld/account-refresh updated
23. By James Henstridge

Convert account watcher to track a single service type rather than a set
of service names. This is needed to properly receive enabled event
notifications.

24. By James Henstridge

Add service files for Facebook and Twitter, and a service type file that
lets us select them along with GMail.

25. By James Henstridge

Fix up a crash when we are reporting errors to Go and no session data is
available.

26. By James Henstridge

Switch to the new prototype for accounts.NewWatcher() and the new
service names for Twitter and Facebook.

27. By James Henstridge

Disable the debug spew from account-watcher.c

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:26
http://jenkins.qa.ubuntu.com/job/account-polld-ci/3/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-amd64-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-armhf-ci/3
        deb: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-armhf-ci/3/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-i386-ci/3

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/account-polld-ci/3/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:27
http://jenkins.qa.ubuntu.com/job/account-polld-ci/4/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-amd64-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-armhf-ci/4
        deb: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-armhf-ci/4/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-i386-ci/4

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/account-polld-ci/4/rebuild

review: Approve (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

I was able to confirm the problem detecting new accounts, and believe I've fixed that in this branch.

It looks like the enabled-event isn't reliably sent out unless the account manager is limited to a single service type. So I've added an account-polld.service-type file along with services for Facebook and Twitter (as discussed during the call on Tuesday). The AccountWatcher now watches for all account services matching "account-polld", so gets notified of the three services we're interested in. This seems to do the trick and simplifies things a bit.

I've also disabled the debug spew from account-watcher.c, while still allowing it to be turned back on if I need to track down some logic bugs.

lp://staging/~jamesh/account-polld/account-refresh updated
28. By James Henstridge

Merge from trunk

29. By James Henstridge

Install additional online accounts files.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:29
http://jenkins.qa.ubuntu.com/job/account-polld-ci/5/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-amd64-ci/5/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-armhf-ci/5/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-i386-ci/5/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/account-polld-ci/5/rebuild

review: Needs Fixing (continuous-integration)
lp://staging/~jamesh/account-polld/account-refresh updated
30. By James Henstridge

Fix typo.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:30
http://jenkins.qa.ubuntu.com/job/account-polld-ci/6/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-amd64-ci/6
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-armhf-ci/6
        deb: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-armhf-ci/6/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-utopic-i386-ci/6

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/account-polld-ci/6/rebuild

review: Approve (continuous-integration)
Revision history for this message
Sergio Schvezov (sergiusens) wrote :

Using service types doesn't seem to work with gmail

Revision history for this message
James Henstridge (jamesh) wrote :

Can you be more specific about what doesn't work? Are you using the updated .service file? (note that I updated the service type here).

When I run "./account-watcher-test account-polld" with the extra files installed, I get auth data for all three of my accounts.

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

I never get the enabled signal for gmail, nor do I see the application toggle in online accounts when testing on the phone (haven't tried on the desktop).

There's also a typo in debian/rules I've made
669 ${CURDIR}/debian/account-polld/usr/share/accounts/application
and the cp is missing a trailing 's'.

But my testing involved installing
http://jenkins.qa.ubuntu.com/job/account-polld-utopic-armhf-ci/6/artifact/work/output/*zip*/output.zip and moving the .application afterwards

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

what I am seeing are most likely online accounts issues which I can reproduce with sync-monitor as well.

review: Approve
Revision history for this message
Sergio Schvezov (sergiusens) wrote :

I'll get this in a silo in the morning; feel free to do so yourself if you want; I'll chain up some MPs similar to this one
https://code.launchpad.net/~sergiusens/account-polld/oa_fixes/+merge/228195

if you don't mind doing them here as well (at least the trailing 's')

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