Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jul 14, 2025

Description

Share and reuse CLI version checking logic with app host.

Note that there are issues sharing internal types between CLI and app host because CLI tests references both and gets duplicate type errors. Fixed by customizing a type name between projects.

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 requested review from Copilot and davidfowl July 14, 2025 03:43
@JamesNK JamesNK added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jul 14, 2025
@JamesNK JamesNK requested a review from mitchdenny as a code owner July 14, 2025 03:43
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR centralizes package version checking by introducing shared helpers and types, replacing separate logic in both the CLI and the hosting app.

  • Added PackageUpdateHelpers and a shared NuGetPackage/NuGetPackageCli type in src/Shared
  • Replaced IVersionFetcher/VersionFetcher with IPackageFetcher/PackageFetcher in hosting code
  • Updated CLI and hosting tests to use TestPackageFetcher and the new shared parsing logic

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

File Description
src/Shared/PackageUpdateHelpers.cs New shared parsing and version comparison utilities
src/Aspire.Hosting/VersionChecking/PackageFetcher.cs New host-side implementation of IPackageFetcher
src/Aspire.Hosting/VersionChecking/VersionCheckService.cs Switched from IVersionFetcher to IPackageFetcher & called shared helpers
tests/Aspire.Hosting.Tests/VersionChecking/VersionCheckServiceTests.cs Refactored tests to use TestPackageFetcher
Comments suppressed due to low confidence (5)

src/Shared/PackageUpdateHelpers.cs:5

  • Add missing using directives for System.Collections.Generic, System.Linq, and System.Threading.Tasks so that List, LINQ methods, and Task types resolve correctly.
using Semver;

src/Shared/PackageUpdateHelpers.cs:113

  • The empty collection literal [] cannot be converted to List. Replace with return new List<NuGetPackage>();.
            return [];

src/Aspire.Hosting/VersionChecking/PackageFetcher.cs:54

  • The empty collection literal [] here should be a new List(), e.g., return new List<NuGetPackage>();.
                return [];

tests/Aspire.Hosting.Tests/VersionChecking/VersionCheckServiceTests.cs:32

  • You need to pass a List to the TaskCompletionSource. Use new List<NuGetPackage> { new NuGetPackage { ... } } instead of the array literal.
        packagesTcs.TrySetResult([new NuGetPackage { Id = PackageFetcher.PackageId, Version = "100.0.0" }]);

tests/Aspire.Hosting.Tests/VersionChecking/VersionCheckServiceTests.cs:228

  • Replace Task.FromResult<List<NuGetPackage>>([]) with Task.FromResult(new List<NuGetPackage>()) to return an empty list.
            _versionTask = versionTask ?? Task.FromResult<List<NuGetPackage>>([]);

@JamesNK JamesNK added this to the 9.4 milestone Jul 14, 2025
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.

Works well!

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.

I need to verify the CLI changes.

@dotnet-policy-service dotnet-policy-service bot added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 14, 2025
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.

Works!

@davidfowl davidfowl merged commit 55a5104 into main Jul 14, 2025
544 of 546 checks passed
@davidfowl davidfowl deleted the jamesnk/version-check-unification branch July 14, 2025 12:48
@davidfowl
Copy link
Member

/backport to release/9.4

Copy link
Contributor

Started backporting to release/9.4: https://github.com/dotnet/aspire/actions/runs/16267295935

@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication needs-author-action An issue or pull request that requires more info or actions from the author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants