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

Refactor UpdateManager & Remove UpdateChecker #13027

Merged
merged 18 commits into from
May 17, 2024

Conversation

Szpoti
Copy link
Collaborator

@Szpoti Szpoti commented May 13, 2024

Better to review commit-by-commit.

As the backend gets more and more disconnected from the client, we can refactor the update workflow as well.

This PR aims to remove UpdateChecker and use mainly the UpdateManager (which got refactored as well).
What happens here:

  • We get the client version from GitHub mainly, removing another backend dependency.
  • This way we only need the BackendMajorVersion from the backend.
  • We can get rid of UpdateChecker this way.
  • Refactor UpdateManager and check for updates only during initialization.

Copy link
Collaborator

@lontivero lontivero left a comment

Choose a reason for hiding this comment

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

It looks very well.

}

public bool ClientUpToDate { get; }
public bool ClientUpToDate { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this?

The whole class should be replaced by something like this:

public record UpdateStatus(bool ClientUpToDate, bool BackendCompatible, bool IsReadyToInstall, Version ClientVersion);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just about to write the same. It would be a great simplification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could simplify it yes, but to note:
When we instantiate UpdateStatus here, we aren't aware of ClientVersion (API request needed), ReadyToInstall (download needed) and ClientUpToDate (client version comparison needed).
I went with this because this way we can set these properties in UpdateManager when its needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Records allow you to set properties:

image

Or was your question about something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kiminuo is this ok? 6edb57b

WalletWasabi/Services/UpdateManager.cs Outdated Show resolved Hide resolved
WalletWasabi/Services/UpdateManager.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

Something I don't get: Does this PR remove the ability for the client to find updates while it's running? It looks from the code that by removing UpdateChecker we can now only catch updates on relaunch
Can we make UpdateManager a PeriodicRunner (with UpdateClientAsync being ActionAsync)?

@Szpoti
Copy link
Collaborator Author

Szpoti commented May 14, 2024

@turbolay Your suggestion was my original idea, @lontivero suggested to go with this one-time-check solution.

@turbolay
Copy link
Collaborator

@lontivero Can you confirm that having the UpdateManager as a PeriodicRunner is a cNACK for you?

I personally dislike the idea of running the check for updates only at launch for a simple reason: It's better to never close Wasabi. It's less true without coinjoining but still. Also I see no harm in having a request per hour to GitHub.

@lontivero
Copy link
Collaborator

Your suggestion was my original idea, @lontivero suggested

Yes, it is true, I suggested that. Well, I have no problem with checking from time to time. However I think this PR is good and I would like to merge it. @Szpoti would you like to check periodically again but i a different PR?

@MaxHillebrand
Copy link
Collaborator

fwiw, I think checking for releases once a day is enough. If we would check every hour, that's almost 1500 unnecessary checks during a two month release cycle. Distributing the new downloads across 24 hours is also a nice load balancing for the server, and in the case of a swift hotfix some users wouldn't download the initial version.

@Szpoti
Copy link
Collaborator Author

Szpoti commented May 17, 2024

@lontivero I can do that if you find it necessary. It can be a 24 hours cycle as Max suggested it.
Also pls reply your idea about this comment: #13027 (comment)

@turbolay turbolay requested a review from kiminuo May 17, 2024 16:51
…to refactorUpdateManager

# Conflicts:
#	WalletWasabi.Daemon/Global.cs
Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

tACK

I pushed a commit for suggestion about making UpdateStatus a record. I also fixed conflicts.

I will merge considering we had several reviews and I only changed style (the equality comparer were not custom). If what I did with the record is not what you had in mind @kiminuo please make another PR.

I will make a new PR about PeriodicRunner

@turbolay turbolay merged commit 0d5dd0d into WalletWasabi:Downsize+FOSS May 17, 2024
3 of 5 checks passed
lontivero pushed a commit that referenced this pull request May 21, 2024
Refactor UpdateManager and remove UpdateChecker
turbolay pushed a commit to turbolay/WalletWasabi that referenced this pull request May 30, 2024
Refactor UpdateManager and remove UpdateChecker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants