Code review comment for lp:~nikwen/account-polld/donechan-fix

Revision history for this message
John Lenton (chipaca) wrote :

Finally have time to review this! Sorry for the delay.

While the bug is real, I have nits with your fix:

the doneChan is a channel for errors, but you're sending nil (no error) for at least two cases that are clearly errors. This needs fixing; I suggest returning instead ad-hoc errors and logging them in a single place (so e.g. returning fmt.Errorf("failed to authenticate: %v", a.authData.Error) instead of log-and-return-nil; then the "has failed:", err in the error path should suffice).

This will have the side effect of bumping penaltyCount for auth errors and click-not-installed, which is not necessarily a bad thing.

review: Needs Fixing

« Back to merge proposal