Skip to content

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

Merged
merged 14 commits into from
Jul 1, 2025
Merged

Add Aspire upgrade check #10034

merged 14 commits into from
Jul 1, 2025

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jun 25, 2025

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

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@JamesNK JamesNK added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jun 25, 2025
@mitchdenny
Copy link
Member

mitchdenny commented Jun 25, 2025

We already take a dependency on SemVersion in the Aspire CLI. You could potentially use it to do the semantic version checks. That way it could work even for prerelease versions (the implication being it takes some of the heavy lift out of doing the version comparisons).

Comment on lines +121 to +122
title: "Update now",
message: $"Aspire {latestVersion} is available.",
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

@adamint adamint Jun 30, 2025

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@mitchdenny
Copy link
Member

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.

@JamesNK
Copy link
Member Author

JamesNK commented Jun 26, 2025

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:

image

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)

@JamesNK
Copy link
Member Author

JamesNK commented Jun 26, 2025

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?

@mitchdenny
Copy link
Member

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.

@davidfowl
Copy link
Member

Should checking the version be moved later so it doesn't distract machine CPU/memory from starting DCP/dashboard/resources?

Yes, move it to later. After starting.

@JamesNK JamesNK force-pushed the jamesnk/version-check branch from 24db38e to be0ccb1 Compare June 27, 2025 01:21
@davidfowl
Copy link
Member

Make the logs all debug. This should never show up for the user.

@JamesNK
Copy link
Member Author

JamesNK commented Jun 27, 2025

Should checking the version be moved later so it doesn't distract machine CPU/memory from starting DCP/dashboard/resources?

Yes, move it to later. After starting.

How do you want to do this? There is AfterResourcesCreatedEvent, but it isn't called until waiting resources start.

Could do something simple like a Task.Delay for a couple of seconds.

@JamesNK JamesNK marked this pull request as ready for review June 27, 2025 06:13
@JamesNK
Copy link
Member Author

JamesNK commented Jun 27, 2025

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));
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

@JamesNK JamesNK Jun 29, 2025

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?

Copy link
Member

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.

Copy link
Member

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...

Copy link
Member

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 cleans. So maybe it shouldn't be used for the storage of "ignore version upgrade" bool? Unless we want to prompt them everytime they clean.

Copy link
Member

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?

Copy link
Member Author

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 cleans. 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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#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?

Copy link
Member Author

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.

Copy link
Member

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.

@davidfowl
Copy link
Member

Authenticated feeds will be a problem but that's not new I guess.

@JamesNK
Copy link
Member Author

JamesNK commented Jun 28, 2025

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.

@mitchdenny
Copy link
Member

By the time the dashboard is running I would assume auth issues are solved and the notification will eventually work

@DamianEdwards
Copy link
Member

@JamesNK we should follow-up RE the banner styling so that it pops more but it's not a ship-blocker.

Comment on lines +121 to +122
title: "Update now",
message: $"Aspire {latestVersion} is available.",
Copy link
Member

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

Copy link
Member

@davidfowl davidfowl left a 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.

@IEvangelist
Copy link
Member

Is this intended for 9.4?

@davidfowl
Copy link
Member

This is in 9.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants