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

Allow WithArgs to accept ReferenceExpressions #4415

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

afscrome
Copy link
Contributor

@afscrome afscrome commented Jun 7, 2024

Update the WithArgs extension method to accept object[] rather than string[]. In particular, this now allows for the use of reference expressions inside WithArgs

.WithArgs(
   "-p",
   resource.PrimaryEndpoint.Property(EndpointProperty.TargetPort)
);

Fixes #4237

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jun 7, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 7, 2024
@davidfowl
Copy link
Member

@afscrome are you going to fix this PR?

@afscrome
Copy link
Contributor Author

Hoping to pick this up over the weekend unless someone else wants to take it before then.

Also spotted earlier today that a similar addition is probably warranted for public static IResourceBuilder<ExecutableResource> AddExecutable(this IDistributedApplicationBuilder builder, string name, string command, string workingDirectory, params string[]? args)

@afscrome
Copy link
Contributor Author

There is a similar issue with WithContainerRuntimeArgs. Whilst the underlying ContainerRuntimeArgsCallbackContext has arguments as a list of objects

public IList<object> Args { get; } = args ?? throw new ArgumentNullException(nameof(args));

ContainerResourceBuilderExtensions.WithContainerRuntimeArgs only accepts a string array

public static IResourceBuilder<T> WithContainerRuntimeArgs<T>(this IResourceBuilder<T> builder, params string[] args) where T : ContainerResource
{
return builder.WithContainerRuntimeArgs(context => context.Args.AddRange(args));
}

Shall I fix that as well?

@afscrome
Copy link
Contributor Author

afscrome commented Jun 14, 2024

Any ideas on how to fix the ambiguity this change introduces between

public static IResourceBuilder<ExecutableResource> AddExecutable(this IDistributedApplicationBuilder builder, string name, string command, string workingDirectory, params string[]? args)
public static IResourceBuilder<ExecutableResource> AddExecutable(this IDistributedApplicationBuilder builder, string name, string command, string workingDirectory, params object[]? args)

I could remove the params from the string[] version, but according to https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-rules.md#signatures that is a breaking change, or does the addition of the params object[] version mitigate that break?

But equally if I remove the params from the object[] version, that would somewhat defeat the point of the addition.

@eerhardt
Copy link
Member

@afscrome - it looks like we were using an older SDK - 8.0.200. I updated to the latest .NET SDK 8.0.302 and the build error should be fixed now. You can overload methods on params arrays of different types. So we should be good.

@afscrome
Copy link
Contributor Author

@eerhardt Can this be merged now? I've resolved the merge conflicts and checks are all passing.

@eerhardt
Copy link
Member

@eerhardt Can this be merged now? I've resolved the merge conflicts and checks are all passing.

Can you add some tests for the new functionality?

@davidfowl
Copy link
Member

@afscrome Are you going to finish this one?

@afscrome
Copy link
Contributor Author

afscrome commented Sep 9, 2024

Updated tests to cover the new scenarios.

@davidfowl davidfowl merged commit b261e94 into dotnet:main Sep 9, 2024
11 checks passed
radical pushed a commit to radical/aspire that referenced this pull request Sep 11, 2024
* Support providing `ReferenceExpression` arguments with `WithArgs()`

* Update src/Aspire.Hosting/ExecutableResourceBuilderExtensions.cs

* Update AddExecutableWithArgs test to include new varients allowign
object array params

---------

Co-authored-by: David Fowler <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 11, 2024
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 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.

Update WithArgs to accept object array
3 participants