-
Notifications
You must be signed in to change notification settings - Fork 110
[Automated] Update API Surface Area #715
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
Conversation
9f6121b
to
70a4875
Compare
2bf7e2b
to
f639789
Compare
f639789
to
abe71a6
Compare
5c1bf72
to
2005439
Compare
2005439
to
c54e007
Compare
@@ -10,7 +10,7 @@ namespace Aspire.Hosting | |||
{ | |||
public static partial class GolangAppHostingExtension | |||
{ | |||
public static ApplicationModel.IResourceBuilder<ApplicationModel.GolangAppExecutableResource> AddGolangApp(this IDistributedApplicationBuilder builder, string name, string workingDirectory, string[]? args = null) { throw null; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change.
To avoid this, we can add a new overload:
public static ApplicationModel.IResourceBuilder<ApplicationModel.GolangAppExecutableResource> AddGolangApp(this IDistributedApplicationBuilder builder, string name, string workingDirectory, string[]? args = null, string[]? buildTags = null) { throw null; }
And make the args
parameter for the old API required.
public static ApplicationModel.IResourceBuilder<ApplicationModel.GolangAppExecutableResource> AddGolangApp(this IDistributedApplicationBuilder builder, string name, string workingDirectory,
string[] args) { throw null; }
{ | ||
public static partial class MinioBuilderExtensions | ||
{ | ||
public static ApplicationModel.IResourceBuilder<ApplicationModel.MinioContainerResource> AddMinioContainer(this IDistributedApplicationBuilder builder, string name, ApplicationModel.IResourceBuilder<ApplicationModel.ParameterResource>? rootUser = null, ApplicationModel.IResourceBuilder<ApplicationModel.ParameterResource>? rootPassword = null, int? port = null) { throw null; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be AddMinio
, not AddMinioContainer
, to be consistent with other resources like AddPostgres
or AddOllama
|
||
public EndpointReference PrimaryEndpoint { get { throw null; } } | ||
|
||
public ParameterResource RootUser { get { throw null; } set { } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the Root
part for RootUser
. Is there a reason for this?
public static ApplicationModel.IResourceBuilder<NodeAppResource> WithNpmPackageInstallation(this ApplicationModel.IResourceBuilder<NodeAppResource> resource, bool useCI = false, System.Action<ApplicationModel.IResourceBuilder<ApplicationModel.NpmInstallerResource>>? configureInstaller = null) { throw null; } | ||
|
||
public static ApplicationModel.IResourceBuilder<NodeAppResource> WithPnpmPackageInstallation(this ApplicationModel.IResourceBuilder<NodeAppResource> resource, string[]? args = null) { throw null; } | ||
public static ApplicationModel.IResourceBuilder<NodeAppResource> WithPnpmPackageInstallation(this ApplicationModel.IResourceBuilder<NodeAppResource> resource, System.Action<ApplicationModel.IResourceBuilder<ApplicationModel.PnpmInstallerResource>>? configureInstaller = null) { throw null; } | ||
|
||
public static ApplicationModel.IResourceBuilder<NodeAppResource> WithYarnPackageInstallation(this ApplicationModel.IResourceBuilder<NodeAppResource> resource, string[]? args = null) { throw null; } | ||
public static ApplicationModel.IResourceBuilder<NodeAppResource> WithYarnPackageInstallation(this ApplicationModel.IResourceBuilder<NodeAppResource> resource, System.Action<ApplicationModel.IResourceBuilder<ApplicationModel.YarnInstallerResource>>? configureInstaller = null) { throw null; } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. We should add a new API instead of changing the surfaces. We can deprecate the old ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fundamentally, the whole install pipeline here is a breaking change as it's moving to using a resource that shows in the dashboard. Since the configureInstaller
is a nullable additional argument, the likelihood of it impacting someone is pretty minimal, since they'd have to be invoking the method in a non-standard way.
Although the diff is showing a bit confusing as the args
array wasn't present in 9.5 (https://github.com/CommunityToolkit/Aspire/blob/v9.5.0/src/CommunityToolkit.Aspire.Hosting.NodeJS.Extensions/api/CommunityToolkit.Aspire.Hosting.NodeJS.Extensions.cs#L19-L23), it was my first iteration on this change, which I merged, but then moved to using a callback for flexibility.
b6e7960
to
e6c87e4
Compare
ad77c37
to
f628d41
Compare
f628d41
to
f630d6c
Compare
Minimum allowed line rate is |
Auto-generated update to the API surface to compare current surface vs latest release. This should only be merged once this surface area ships in a new release.