Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor AspireStore APIs #7626

Merged
merged 8 commits into from
Feb 19, 2025
Merged

Refactor AspireStore APIs #7626

merged 8 commits into from
Feb 19, 2025

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Feb 15, 2025

Description

Reacting to https://github.com/dotnet/aspire/pull/7374/files#r1950365868

  • Simplified IAspireStore by moving one method to extensions
  • Replaced extension method by standard static method to create a store
  • Retrieve IAspireStore from DI

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?

@Copilot Copilot bot review requested due to automatic review settings February 15, 2025 02:04
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.

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

Files not reviewed (1)
  • src/Aspire.Hosting/Aspire.Hosting.csproj: Language not supported

@danmoseley danmoseley added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Feb 18, 2025
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This is looking nice now. Looks good. I just had a few minor comments to clean up.

sebastienros and others added 3 commits February 19, 2025 13:09
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@danmoseley
Copy link
Member

Can we get this into 9.1 build tonight..?

@sebastienros
Copy link
Member Author

/backport to release/9.1

Copy link
Contributor

Started backporting to release/9.1: https://github.com/dotnet/aspire/actions/runs/13423606060

@eerhardt
Copy link
Member

Merging. The dotnet.aspire check seems to be stuck, but it passed on the backport, so assuming it is fine. All other builds/tests passed.

@sebastienros sebastienros reopened this Feb 19, 2025
@eerhardt eerhardt merged commit c8ded63 into main Feb 19, 2025
132 of 135 checks passed
@eerhardt eerhardt deleted the sebros/store branch February 19, 2025 23:03
# for free to join this conversation on GitHub. Already have an account? # 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.

4 participants