Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wip] snooze rpc #23305

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[wip] snooze rpc #23305

wants to merge 5 commits into from

Conversation

mmou
Copy link
Contributor

@mmou mmou commented Mar 27, 2020

i added a "snooze update" RPC
on linux, this command will snooze all updates forever (it sets the snooze_updates boolean in config.json).
on darwin/windows, it will snooze the most recent update (if it exists) for 24 hrs.
i also added a CLI command (keybase update snooze)

BLOCKERS:
need to rebase on https://github.com/keybase/client/pull/21591/files
blocked on keybase/go-updater#195

@@ -32,3 +32,8 @@ func StartUpdateIfNeeded(context.Context, logger.Logger) error {
func GetNeedUpdate() (bool, error) {
return false, nil
}

// SnoozeUpdate will snooze the new update (if there is one) for 24 hrs.
func SnoozeUpdate() error {
Copy link
Contributor Author

@mmou mmou Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for linux, it was discussed (cc @heronhaye ?) we might want to be able to snooze indefinitely

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe (jacob knows more about this) that this snoozing is done serverside. Should probably be handled clientside for linux. Could just set it in config.json maybe. Ideally there would be an undo button in the GUI as well but not a priority.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of things here.

  • Snoozing is done on the server for macOS and Windows clients since they have update binaries bundled.

  • Linux client have never snoozed historically because they don't have installIDs. So calling directly to the API server here implicitly depends on an installID. We wouldn't want to do that.

  • It's correct to call into the updater on macOS and Windows since it will handle/manage installIDs.

  • @heronhaye is correct that on Linux this 'snoozing' can only happen client side since we can't depend on the install or update binaries being present.

  • service/config.go would want to perform a platform check (for linux), and instead of calling into install we would call something else that would write to config.json with the end snooze timestamp (epoch?). Subsequent checks for updates on Linux (handled in PICNIC-664) should read from config.json to see if we are before the snooze timestamp. Checking for an update after the snooze timestamp should clear the snooze setting automatically. This discussion thread can be carried into PICNIC-664.

  • 'Undo' needs to go through design. Instead we can add a button to the GUI under settings that says 'Check for update'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine. The install package already does the platform check so you can just put the config write here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great

if err != nil {
return err
}
exec.Command(updaterPath, "snooze")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmou mmou requested review from joshblum, jryio, heronhaye and cjb and removed request for joshblum March 27, 2020 02:21
Copy link
Contributor

@heronhaye heronhaye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux

@mmou mmou force-pushed the mmou/PICNIC-684-snooze branch from 0076214 to b2520d4 Compare March 30, 2020 23:28
@@ -392,6 +392,8 @@ func (m MetaContext) switchUserNewConfig(u keybase1.UID, n NormalizedUsername, s
return err
}
}
// TODO: do i need to do anything particular for snooze_updates when user
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do i need to do anything particular for snooze_updates when user switching?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that updater settings is per-install and not per-user but @thebearjew should confirm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually don't know the answer to this off the top of my head. I'll look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants