-
Notifications
You must be signed in to change notification settings - Fork 652
Add Aspire upgrade check #10034
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 Aspire upgrade check #10034
Conversation
We already take a dependency on |
title: "Update now", | ||
message: $"Aspire {latestVersion} is available.", |
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.
Localization? I was thinking we would use aka.ms/aspire/update as the URL? I need to use it in the CLI too so it makes sense for them to be the same.
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.
Localization is going to be difficult. We're in the app host so we don't know the culture of the browser (or CLI, or VS Code).
I think the best we can do is have localized resources in the app host, and use the OS language to provide the best fit.
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.
We could do what we did for the default commands - try to localize in the dashboard, not here, based on the title
Alternative approach, add a new type like LocalizableString(string Message, string LocKey)
and replace all the user-facing strings like LinkText
below or title
) with it, then look up the loc key in the dashboard/cli
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.
There are args in the text. And there is going to be text from many integrations. That approach doesn't scale.
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.
What would you suggest?
we have to figure out loc before merging
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.
This looks good to me. Tried it out locally and it seems to work. I wonder if the grey banner is too subtle but that is probably more a @DamianEdwards and @maddymontaquila question about how hard they want to push folks to update. |
The message bar uses the built-in FluentUI control. There are limit options on message bar colors and no equivilent of VS's gold bar: If we want another color/icon then either some hacks will be required (I think we can add one more style), or we ask FluentUI people to add new options, or we replace the control with our own one (which I don't want to do) |
When is the best time to run this? Currently it is in a background service and will execute in parallel with the app starting. Should checking the version be moved later so it doesn't distract machine CPU/memory from starting DCP/dashboard/resources? |
I can't imagine it being that resource intensive, and you don't run it all the time. I wouldn't bother trying to optimise that unless we see it being a problem. Regarding the banner colour - bummer, I didn't realise it was limited that way. |
Yes, move it to later. After starting. |
24db38e
to
be0ccb1
Compare
Make the logs all debug. This should never show up for the user. |
How do you want to do this? There is Could do something simple like a Task.Delay for a couple of seconds. |
What should the message link to? I'll add an aka.ms but it needs to be configured to go somewhere |
SemVersion? latestVersion = null; | ||
if (checkForLatestVersion) | ||
{ | ||
SecretsStore.TrySetUserSecret(_options.Assembly, CheckDateKey, now.ToString("o", CultureInfo.InvariantCulture)); |
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.
Do we want to use user-secrets for this? It feels like clutter to the user. What about a file in IAspireStore
instead? The developer typically doesn't see the values in there.
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.
I don't know enough about app host/intergration development to say the pros vs cons.
@davidfowl @mitchdenny @eerhardt What do you think should be used here?
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.
Yeah user secrets is for the user to store user-specific config for the app, not the app to store config for itself. We should use IAspireStore
.
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.
I looked at IAspireStore
and it's for managing files. Not getting and setting name/values.
Do you want it to be extended with APIs for get and set values from a file? e.g. deserialize and serialize to a known JSON format and the file is save to IAspireStore
?
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.
I could imagine the AppHost managing a single "metadata" file where we could put more of this kind of information in the future. Similar to the .aspire/settings.json
file for the CLI, but for the AppHost itself.
The IAspireStore
would only be responsible for giving the BasePath
directory of the store. Something else (probably internal) would know the file name, and how to (de)serialize information in and out of the metadata/settings file.
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.
All of this work to avoid a version in user secrets...
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.
Note that the IAspireStore directory is meant to be cleaned up when someone git clean
s. So maybe it shouldn't be used for the storage of "ignore version upgrade" bool? Unless we want to prompt them everytime they clean.
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.
All of this work to avoid a version in user secrets...
Do we want to turn it into a dumping ground?
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.
Note that the IAspireStore directory is meant to be cleaned up when someone
git clean
s. So maybe it shouldn't be used for the storage of "ignore version upgrade" bool? Unless we want to prompt them everytime they clean.
We don't want to reset the ignore version or last version check date on git clean.
} | ||
} | ||
|
||
#pragma warning restore ASPIREINTERACTION001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. |
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.
#pragma warning restore ASPIREINTERACTION001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. |
What's the value in restoring at the end of the file?
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.
The sense of satisfaction of a job well done.
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.
It just seems like clutter to me, with no value.
Authenticated feeds will be a problem but that's not new I guess. |
Default behavior is not to be interactive. I assume feeds that there isn't already auth setup for will just not be called or fail. |
By the time the dashboard is running I would assume auth issues are solved and the notification will eventually work |
@JamesNK we should follow-up RE the banner styling so that it pops more but it's not a ship-blocker. |
title: "Update now", | ||
message: $"Aspire {latestVersion} is available.", |
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.
We could do what we did for the default commands - try to localize in the dashboard, not here, based on the title
Alternative approach, add a new type like LocalizableString(string Message, string LocKey)
and replace all the user-facing strings like LinkText
below or title
) with it, then look up the loc key in the dashboard/cli
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.
We can file an issue as a follow up to find a new place to store the version information once we've found it.
Is this intended for 9.4? |
This is in 9.4 |
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Checklist
<remarks />
and<code />
elements on your triple slash comments?doc-idea
templatebreaking-change
templatediagnostic
template