Report action to API #40
Conversation
The gocyclo score for updater.Update() got too high so had to reduce it a little
|
@keybase/updater-hackers bump |
# The first commit's message is: Report action to API The gocyclo score for updater.Update() got too high so had to reduce it a little # This is the 2nd commit message: Add tests
| case UpdateActionApply: | ||
| ctx.ReportAction(UpdateActionApply, options) | ||
| case UpdateActionAuto: | ||
| ctx.ReportAction(UpdateActionAuto, options) |
There was a problem hiding this comment.
Is this when they chose to apply the update, and also have clicked the "make next update auto" button?
There was a problem hiding this comment.
auto is if the update was applied automatically without user prompt...
so the first time the action will be apply, and if they checked automatic, then next time it would be auto
There was a problem hiding this comment.
OK. The way it's currently implemented, if you send apply then the auto bit is set to false, and if you send auto, the auto bit is set to true:
https://github.com/keybase/keybase/pull/647/files#diff-09c19ea087354ad87b44929d4a676ff1R224
There was a problem hiding this comment.
is it important to distinguish between... this update was applied without user being prompted... in which case my reporting is correct... if the goal is to report the "auto" setting then we need to change this request to not mix an action with state?
So params would be?
action: "apply", "snooze", "cancel", "auto"
auto_update: 1/0
If apply is chosen and user doesn't enable auto: action=apply&auto_update=0
If apply is chosen and user turns on auto: action=apply&auto_update=1
If there was no prompt: action=auto&auto_update=1
If we ever see auto with auto_apply=0 then that would indicate a problem.
There was a problem hiding this comment.
I'm fine with this change, though I would have predicted that you wouldn't like a 2x2 of bools in which one quadrant is undefined behavior :)
There was a problem hiding this comment.
Ok, changed the pseudocode and confluence doc to match. BTW, I never implemented cancel here.
There was a problem hiding this comment.
Ok, changes are up here: https://github.com/keybase/keybase/pull/647/commits/1861e3d0fd9f13dbda7f70037ac27ad6efc5e877
There was a problem hiding this comment.
Should I remove reporting of cancel? It's probably not useful. Only happens in case of prompt timeout.
There was a problem hiding this comment.
I can report it and the server can ignore it if it wants to?
|
A few nits. Looks good otherwise. |
|
Gonna add async reporting and auto_update in separate PRs. |
The gocyclo score for updater.Update() got too high so had to reduce it a little.
@keybase/updater-hackers