feat(manager): prompt user to update manual server #2409
Conversation
fortuna
left a comment
fortuna
left a comment
There was a problem hiding this comment.
What action do we expect from the user? Servers are auto-updated, even the ones installed with the script.
Somebody (hopefully not @sbruens) is going to write an article that this will link to. |
|
An article to do what? The provider doesn’t need (or should) manually upgrade the server, unless they used outline-ss-server directly, in which case they don’t use the manager. Am I missing anything? |
sbruens
left a comment
sbruens
left a comment
There was a problem hiding this comment.
The main comment I have is on building in a delay for these notifications. Watchtower updates check every hour IIRC, and for users on auto-update (the majority of users) there is actually nothing they should do. I don't think those users should ever see this notification because it will create confusion. The only way I think we can detect that (without adding the check to the server side) is by adding in some delay.
So you're saying we should wait until every hour on the hour before making this check? Or maybe on the quarter hour? |
It's not necessarily on the hour, but it just polls every hour (see https://containrrr.dev/watchtower/arguments/#poll_interval). I think there should just be a delay. If we check now and the server requires an update, we should try again in at least an hour and if it's still required, then auto-update through watchtower didn't upgrade the image. We can be more liberal and maybe make it a few hours. |
|
Or check the image information from quay.io to see how old the image is and base it on that timestamp, if that's easier. Again, if it's a few hours old, then we know there wasn't an auto-update. That might actually be easier to implement. |
| ); | ||
|
|
||
| if (!(response.status === 200 && response.ok)) { | ||
| return Promise.reject(response.statusText); |
There was a problem hiding this comment.
Only throw errors: https://google.github.io/styleguide/tsguide.html#exception-handling
There was a problem hiding this comment.
Let me set this up in ESLint.
| } | ||
|
|
||
| // A tag indicating a version of the server code | ||
| export interface VersionTag { |
There was a problem hiding this comment.
Remove duplicate interface?
| async getLatestAvailableUpdate(): Promise<server.VersionTag> { | ||
| const recentVersionTags = await fetchRecentShadowboxVersionTags(); | ||
|
|
||
| const latestVersionTag = recentVersionTags.find( |
There was a problem hiding this comment.
Looking at the quay documentation, I'm not sure we want to just blindly grab the latest numbered version. It assumes the latest version is good. That could be a problem if the highest version is not stable, e.g. in the event of a rollback.
It would be slightly better to have the logic look at the active stable tag and find the version name for that one, which you can cross-reference by manifest_digest. For example, from this query you can see that the latest stable version is 1.12.3.
$ curl -s "https://quay.io/api/v1/repository/outline/shadowbox/tag/?onlyActiveTags=true" | jq -r '.tags[] | "\(.manifest_digest) \(.name)"'
sha256:545c6f7c7261bb30ae1dffe24a6fca5f8512f5d17c72cfb9e410e7e655444e62 stable
sha256:545c6f7c7261bb30ae1dffe24a6fca5f8512f5d17c72cfb9e410e7e655444e62 v1.12.3
sha256:545c6f7c7261bb30ae1dffe24a6fca5f8512f5d17c72cfb9e410e7e655444e62 canary
sha256:545c6f7c7261bb30ae1dffe24a6fca5f8512f5d17c72cfb9e410e7e655444e62 v1.12.3-rc1
sha256:6f430dcce690684085e9445421146e8622c48bb9d3b0dec3c8bd9bbfad6daf01 v1.12.2
sha256:6f430dcce690684085e9445421146e8622c48bb9d3b0dec3c8bd9bbfad6daf01 v1.12.2-rc2
sha256:6cf1296d4d3ac715c530868e351e474cd302a1e56b55aacf14f1d5ee02a44f2d v1.12.2-rc1
...
There was a problem hiding this comment.
Ah, yeah I was worried about that. Let me think about the best way to do this, as per @fortuna's suggestion.
| getAccessKey(accessKeyId: AccessKeyId): Promise<AccessKey>; | ||
|
|
||
| // Return version tag of latest available update | ||
| getLatestAvailableUpdate?: () => Promise<VersionTag>; |
There was a problem hiding this comment.
Why is this optional? This can start making the interface complex. I'd rather make all properties required. Alternatively (and perhaps preferably), declare it only on the ManualServer interface.
There was a problem hiding this comment.
Because I was being lazy. I'll make it required.
There was a problem hiding this comment.
This is adding a fresh new dependency to Quay.io, which goes against our goal to not be tied to them.
I recommend against using their api. Use the docker api instead.
Alternatively, we could publish the latest somewhere that we can simply fetch with https.
Co-authored-by: Sander Bruens <sbruens@users.noreply.github.com>
Co-authored-by: Sander Bruens <sbruens@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.