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

Add framework for an update checker and prompter #688

Merged
merged 13 commits into from
Mar 28, 2019
Merged

Conversation

gerboland
Copy link
Contributor

An UpdatePrompt implementation decides if an update prompt should be shown to the user, and will populate the gRPC UpdateInfo struct with update information.

GithubUpdatePrompt spins up a thread to check for updates every 24 hours, and will prompt the user that an update is available every 6 hours.

DisabledUpdatePrompt is for platforms that have their own update mechanism that we use. Since on Linux we use Snaps, this is used.

Copy link
Collaborator

@Saviq Saviq left a comment

Choose a reason for hiding this comment

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

We're trying to go for semantic versioning, so the target version scheme would be MAJOR.MINOR.PATCH, also to simplify - if there's any suffix, it's a lower version. -pre wouldn't be anything special.

And I think we can ignore the YEAR.MONTH... version scheme as we've abandoned it and the updater would never see them?

So I'd order the (relevant) version numbers like so (newest to oldest):

v1.0.0
v1.0.0-blah
v0.6.0
v0.6.0-pre1
v0.5

WDYT?

@gerboland
Copy link
Contributor Author

We're trying to go for semantic versioning, so the target version scheme would be MAJOR.MINOR.PATCH, also to simplify - if there's any suffix, it's a lower version. -pre wouldn't be anything special.

And I think we can ignore the YEAR.MONTH... version scheme as we've abandoned it and the updater would never see them?

So I'd order the (relevant) version numbers like so (newest to oldest):

v1.0.0
v1.0.0-blah
v0.6.0
v0.6.0-pre1
v0.5

WDYT?

Yep, ok.

Copy link
Collaborator

@Saviq Saviq left a comment

Choose a reason for hiding this comment

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

Hmm this seems to do the checking on the client side, I thought we'd do this in the daemon and push through the RPC… I know both sides may actually be up for upgrade, so we may need both in the long run…

Seems there's a bunch of formatting-only changes, would be good if you undid them.

src/client/cmd/common_cli.cpp Outdated Show resolved Hide resolved
src/client/cmd/common_cli.cpp Outdated Show resolved Hide resolved
src/client/cmd/list.cpp Outdated Show resolved Hide resolved
src/client/cmd/start.cpp Outdated Show resolved Hide resolved
src/client/cmd/version.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@Saviq Saviq left a comment

Choose a reason for hiding this comment

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

Yep, ok.

Then again, we rely on git describe to name versions, so a stable v0.6.0 would be treated as an update on v0.6.0-1-g123456, which would be a later commit… Maybe we should go the dpkg route... (alphabetical, with ~ lower than nil)…

src/platform/update/new_release_monitor.h Outdated Show resolved Hide resolved
@Saviq
Copy link
Collaborator

Saviq commented Mar 19, 2019

Hmm this seems to do the checking on the client side, I thought we'd do this in the daemon and push through the RPC… I know both sides may actually be up for upgrade, so we may need both in the long run…

Scratch that, missed the reply handling in common_cli

src/client/cmd/launch.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@ricab ricab left a comment

Choose a reason for hiding this comment

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

This is a partial review only, but committing as I have already a few comments and requests.

I like the separation of the update_prompt implementations and that the check is offloaded to a different thread. However, I am concerned with the safety of the current mechanism and have some suggestions to improve clarity.

src/platform/update/new_release_monitor.cpp Outdated Show resolved Hide resolved
src/platform/update/new_release_monitor.cpp Outdated Show resolved Hide resolved
src/platform/update/new_release_monitor.cpp Show resolved Hide resolved
src/platform/update/new_release_monitor.cpp Show resolved Hide resolved
src/platform/update/new_release_monitor.cpp Show resolved Hide resolved
src/platform/update/github_update_prompt.cpp Outdated Show resolved Hide resolved
src/platform/update/github_update_prompt.cpp Outdated Show resolved Hide resolved
src/platform/update/version.h Outdated Show resolved Hide resolved
src/platform/update/version.cpp Outdated Show resolved Hide resolved
src/platform/update/version.h Outdated Show resolved Hide resolved
@gerboland
Copy link
Contributor Author

Build will fail until we actually adopt semantic versioning

@Saviq
Copy link
Collaborator

Saviq commented Mar 27, 2019

Build will fail until we actually adopt semantic versioning

We did already :)

I fixed a tiny conflict in test_daemon.cpp.

@Saviq
Copy link
Collaborator

Saviq commented Mar 27, 2019

From CI:

/root/project/parts/multipass/src/src/platform/update/new_release_monitor.cpp:142:35: fatal error: new_release_monitor.moc: No such file or directory

@codecov
Copy link

codecov bot commented Mar 27, 2019

Codecov Report

Merging #688 into master will decrease coverage by 0.08%.
The diff coverage is 59.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #688      +/-   ##
==========================================
- Coverage   66.59%   66.51%   -0.09%     
==========================================
  Files         168      174       +6     
  Lines        5943     6041      +98     
==========================================
+ Hits         3958     4018      +60     
- Misses       1985     2023      +38
Impacted Files Coverage Δ
src/client/cmd/common_cli.h 100% <ø> (ø) ⬆️
include/multipass/update_prompt.h 0% <0%> (ø)
src/platform/update/disabled_update_prompt.h 0% <0%> (ø)
src/platform/platform_linux.cpp 34.61% <0%> (-2.89%) ⬇️
include/multipass/qt_delete_later_unique_ptr.h 100% <100%> (ø)
src/platform/update/new_release_monitor.h 100% <100%> (ø)
include/multipass/new_release_info.h 100% <100%> (ø)
src/client/cmd/common_cli.cpp 72% <14.28%> (-22.45%) ⬇️
src/client/cmd/list.cpp 96.96% <50%> (-3.04%) ⬇️
src/daemon/daemon_config.cpp 50% <66.66%> (ø) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3febdd7...465a9cb. Read the comment docs.

@gerboland
Copy link
Contributor Author

Small change: moved all version string parsing into a single block, and catch any semver parse error. If either current version or update version string is not of the correct format, it will just print a warning (it used to crash)

@gerboland gerboland requested a review from ricab March 27, 2019 15:46
ricab
ricab previously requested changes Mar 27, 2019
Copy link
Collaborator

@ricab ricab left a comment

Choose a reason for hiding this comment

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

Neat. Great approach overall. A few requests and questions below.

3rd-party/CMakeLists.txt Show resolved Hide resolved
tests/CMakeLists.txt Show resolved Hide resolved
src/rpc/multipass.proto Show resolved Hide resolved
src/platform/update/github_update_prompt.h Outdated Show resolved Hide resolved
src/platform/update/new_release_monitor.h Show resolved Hide resolved
tests/test_new_release_monitor.cpp Outdated Show resolved Hide resolved
tests/test_new_release_monitor.cpp Outdated Show resolved Hide resolved
src/daemon/daemon.cpp Show resolved Hide resolved
src/daemon/daemon.cpp Show resolved Hide resolved
src/daemon/daemon.cpp Show resolved Hide resolved
ricab added 2 commits March 28, 2019 17:32
Improves formatting, due to heading newline, as well as consistency
with other commands.
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

Ok, I think we're at the point where we can merge, so let's go for it!

bors r=ricab,Saviq,townsend2010

bors bot added a commit that referenced this pull request Mar 28, 2019
688: Add framework for an update checker and prompter r=ricab,Saviq,townsend2010 a=gerboland

An UpdatePrompt implementation decides if an update prompt should be shown to the user, and will populate the gRPC UpdateInfo struct with update information.

GithubUpdatePrompt spins up a thread to check for updates every 24 hours, and will prompt the user that an update is available every 6 hours.

DisabledUpdatePrompt is for platforms that have their own update mechanism that we use. Since on Linux we use Snaps, this is used.



Co-authored-by: Gerry Boland <[email protected]>
Co-authored-by: Michał Sawicz <[email protected]>
Co-authored-by: Ricardo Abreu <[email protected]>
@bors
Copy link
Contributor

bors bot commented Mar 28, 2019

Build failed

@Saviq
Copy link
Collaborator

Saviq commented Mar 28, 2019

Expected failure on macOS.

@Saviq Saviq dismissed ricab’s stale review March 28, 2019 22:14

Approved in comments.

@Saviq Saviq merged commit 465a9cb into master Mar 28, 2019
@bors bors bot deleted the update-checker branch March 28, 2019 22:16
Saviq added a commit that referenced this pull request Mar 29, 2019
688: Add framework for an update checker and prompter r=ricab,Saviq,townsend2010 a=gerboland

An UpdatePrompt implementation decides if an update prompt should be shown to the user, and will populate the gRPC UpdateInfo struct with update information.

GithubUpdatePrompt spins up a thread to check for updates every 24 hours, and will prompt the user that an update is available every 6 hours.

DisabledUpdatePrompt is for platforms that have their own update mechanism that we use. Since on Linux we use Snaps, this is used.



Co-authored-by: Gerry Boland <[email protected]>
Co-authored-by: Michał Sawicz <[email protected]>
Co-authored-by: Ricardo Abreu <[email protected]>
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.

5 participants