-
Notifications
You must be signed in to change notification settings - Fork 492
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
Make UpdateManager
a PeriodicRunner
#13050
Make UpdateManager
a PeriodicRunner
#13050
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK.
Do you think it makes sense to register UpdateManager as a Hosted Service now it's a periodic runner?
I thought about it while making this PR but I don't think so. If we need it somewhere or to stop it or whatever in the future well we can register it. If someone thinks differently, I will update this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It makes sense to have this because some users are just running the software without restart. They might not even look at the screen but if they do at least they will see the update message.
Regarding the disposal... UpdateManager
is IDisposable
- so you need to do that somewhere. Right now it is missing.
There are two options:
- Add it to
HostedServices
. - Add the disposal to
Global.DisposeAsync
.
@lontivero I can see you forced push here, what happened to this PR? |
8e0d95e
to
681d785
Compare
Code looks good to me but this line fails for me: and I get always "Not Found":
This makes me think that the endpoint got changed by a Lucas PR (there was a PR changing API endpoints). But I don't know for sure. |
@kiminuo The endpoint was changed on this branch, and it's not deployed yet, which explains why it doesn't reach. I would like to merge this PR so I can make a new PR to merge to master. I will take care to change the endpoint back to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot test it, becasue the endpoint is missing. But the code LGTM.
Period = 1 day
according to #13027 (comment)