- 
                Notifications
    
You must be signed in to change notification settings  - Fork 133
 
[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
  
    | { | ||
| 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.