Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

allow auto update with check command #161

Merged
merged 3 commits into from
Apr 20, 2018

Conversation

zanderz
Copy link
Contributor

@zanderz zanderz commented Apr 19, 2018

No description provided.

@zanderz zanderz requested review from cjb and mlsteele April 19, 2018 23:30
@mlsteele
Copy link
Contributor

What is autoOverride? I see that it comes from config struct. Is it only for tests?

updater.go Outdated
@@ -129,6 +129,8 @@ func (u *Updater) update(ctx Context, options UpdateOptions) (*Update, error) {
return update, promptErr(fmt.Errorf("Unknown prompt error"))
case UpdateActionContinue:
// Continue
case UpdateActionUIBusy:
// Continue (no need to report this to the server)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the point of finding out UIBusy is to not do the update yet. So this should be more like case UpdateActionError, right?

updater.go Outdated
}
if err != nil {
return UpdateActionError, err
}
Copy link
Contributor

@mlsteele mlsteele Apr 20, 2018

Choose a reason for hiding this comment

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

Swap isActive and err checks.

(I edited this comment, sorry for the confusion)

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you've changed checkUserActive to actually return errors instead of just returning err=nil all the time. If that thing fails the proper response is probably to go ahead and update. A faulty gui writing bogus app-state files shouldn't block upgrades forever.

What about something like this where we just ignore errors from checkUserActive?

isActive, err := u.checkUserActive(ctx)
if err == nil && isActive {
    return UpdateActionUIBusy, nil
}
u.guiBusyCount == 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah of course, sorry. I added to the test for this case.

@zanderz
Copy link
Contributor Author

zanderz commented Apr 20, 2018

autoOverride is kind of a Windows leftover from when we attempted to detect whether the update contained new drivers, in which case silent update had to be skipped.

@@ -15,6 +15,8 @@ const (
CancelError ErrorType = "cancel"
// ConfigError is for errors reading/saving config
ConfigError ErrorType = "config"
// ConfigError is for when the GUI is active
GUIBusyError ErrorType = "guiBusy"
Copy link
Contributor

Choose a reason for hiding this comment

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

Now this dude's unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used in IsGuiBusy() to test for this case in report()

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah my bad

Copy link
Contributor

@mlsteele mlsteele left a comment

Choose a reason for hiding this comment

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

Looks good!

@zanderz zanderz merged commit 2fc6a58 into master Apr 20, 2018
@zanderz zanderz deleted the zanderz/DESKTOP-6553/updater_prompt branch April 20, 2018 22:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants