Skip to content
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 GetArgumentListAsync to resource extentions #6646

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Alirexaa
Copy link
Contributor

@Alirexaa Alirexaa commented Nov 11, 2024

Description

This pull request introduces a new method to retrieve arguments from resources asynchronously and includes updates to the public API and tests to support this new functionality.

New Method for Argument Retrieval:

  • Added GetArgumentListAsync method to ResourceExtensions to retrieve arguments from resources asynchronously. This method handles different types of arguments, including those provided by IValueProvider. (src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs)

Public API Update:

  • Updated PublicAPI.Unshipped.txt to include the new GetArgumentListAsync method in the public API. (src/Aspire.Hosting/PublicAPI.Unshipped.txt)

Unit Tests:

  • Added a new test GetArgumentListAsyncReturnCorrectArguments to verify that GetArgumentListAsync returns the correct arguments. (tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs)
  • Introduced a DummyResource class implementing IResourceWithArgs for testing purposes. (tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs)

Fixes #6644 6644

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?
Microsoft Reviewers: Open in CodeFlow

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Nov 11, 2024
/// </remarks>
/// <param name="resource">The resource to get the arguments from.</param>
/// <returns>The arguments retrieved from the resource.</returns>
public static async ValueTask<IReadOnlyList<string>> GetArgumentListAsync(this IResourceWithArgs resource)
Copy link
Member

Choose a reason for hiding this comment

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

Are there any places in the current code that can be using this new API now? I see a bunch of places where we already have this logic. Especially in tests.

Copy link
Contributor Author

@Alirexaa Alirexaa Nov 12, 2024

Choose a reason for hiding this comment

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

Yes, there are. Do you want me to update those codes that use this new API or can I do this in another PR?

Copy link
Member

Choose a reason for hiding this comment

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

Please do it here. It adds extra validation to the API.

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.

LGTM. Thanks for the contribution, @Alirexaa.

@mitchdenny @davidfowl - any concerns?

@davidfowl
Copy link
Member

Yea I think we need to unify these methods (this and GetEnvironmentVariables async) with what happens at runtime. I'm guessing you're using this for testing, but it is missing some critical functionality that we added in 9.

var task = ExpressionResolver.ResolveAsync(isContainer, valueProvider, DefaultContainerHostName, cancellationToken);

@Alirexaa
Copy link
Contributor Author

@davidfowl @eerhardt
I'm not familiar with those changes that were added in 9.
Could you edit my PR to capture those?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an API to easily extract a resource's arguments
3 participants