[Automated] Update API Surface Area #7811

github-actions · 2025-02-27T16:16:04Z

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.

src/Aspire.Hosting.Kubernetes/api/Aspire.Hosting.Kubernetes.cs

src/Aspire.Cli/api/aspire.cs

davidfowl · 2025-03-20T05:56:39Z

src/Aspire.Hosting.Azure.AppContainers/api/Aspire.Hosting.Azure.AppContainers.cs

+        public BicepOutputReference ContainerAppDomain { get { throw null; } }
+
+        public BicepOutputReference ContainerAppEnvironmentId { get { throw null; } }
+
+        public BicepOutputReference ContainerAppEnvironmentName { get { throw null; } }
+
+        public BicepOutputReference ContainerRegistryManagedIdentityId { get { throw null; } }
+
+        public BicepOutputReference ContainerRegistryUrl { get { throw null; } }
+
+        public BicepOutputReference LogAnalyticsWorkspaceId { get { throw null; } }
+
+        public BicepOutputReference ManagedIdentityId { get { throw null; } }
+
+        public BicepOutputReference PrincipalName { get { throw null; } }


I don't think these should be exposed.

davidfowl · 2025-03-20T05:57:11Z

src/Aspire.Hosting.Azure/api/Aspire.Hosting.Azure.cs

@@ -120,6 +120,13 @@ public partial interface IAzureResource : IResource

 namespace Aspire.Hosting.Azure
 {
+    public partial class AppliedRoleAssignmentsAnnotation : ApplicationModel.IResourceAnnotation
+    {
+        public AppliedRoleAssignmentsAnnotation(System.Collections.Generic.HashSet<RoleDefinition> roles) { }


ISet? @eerhardt

I wanted someone else's eyes on how I'm exposing these collections across these new annotations. Here's what we have today:

class AppliedRoleAssignmentsAnnotation

HashSet<RoleDefinition> Roles

class RoleAssignmentAnnotation

IReadOnlySet<RoleDefinition> Roles

class DefaultRoleAssignmentsAnnotation

IReadOnlySet<RoleDefinition> Roles

The reason I made AppliedRoleAssignmentsAnnotation mutable is because we loop over the RoleAssignmentAnnotations and add them into the singular AppliedRoleAssignmentsAnnotation. That allows for simplier code in the resources - they only need to get 1 AppliedRoleAssignmentsAnnotation and not loop over them all. The other 2 feel more like read only is appropriate.

This ones need more explanation. I think I understand the second and third bullet, what is AppliedRoleAssignmentsAnnotation?

src/Aspire.Hosting.Azure/api/Aspire.Hosting.Azure.cs

src/Aspire.Hosting.Docker/api/Aspire.Hosting.Docker.cs

src/Aspire.Hosting/api/Aspire.Hosting.cs

davidfowl · 2025-03-20T06:01:46Z

src/Aspire.Hosting/api/Aspire.Hosting.cs

@@ -697,6 +796,13 @@ public ContainerRuntimeArgsCallbackContext(System.Collections.Generic.IList<obje
        public System.Threading.CancellationToken CancellationToken { get { throw null; } }
    }

+    public sealed partial class CreationScriptAnnotation : IResourceAnnotation


@sebastienros Why is this in Aspire.Hosting?

This can be used by any resource. Other option is to duplicate it in each database integration that needs it. Third option?

Duplicate it in each database resource. Probably with a unique name.

IEvangelist · 2025-03-20T13:41:24Z

src/Aspire.Hosting/api/Aspire.Hosting.cs

+        public static ApplicationModel.IResourceBuilder<TResource> WithHttpCommand<TResource>(this ApplicationModel.IResourceBuilder<TResource> builder, string path, string displayName, System.Func<ApplicationModel.EndpointReference>? endpointSelector, System.Net.Http.HttpMethod? method = null, string? httpClientName = null, System.Func<ApplicationModel.HttpCommandRequestContext, System.Threading.Tasks.Task>? configureRequest = null, System.Func<ApplicationModel.HttpCommandResultContext, System.Threading.Tasks.Task<ApplicationModel.ExecuteCommandResult>>? getCommandResult = null, string? commandName = null, System.Func<ApplicationModel.UpdateCommandStateContext, ApplicationModel.ResourceCommandState>? updateState = null, string? displayDescription = null, string? confirmationMessage = null, string? iconName = null, ApplicationModel.IconVariant? iconVariant = null, bool isHighlighted = false)
+            where TResource : ApplicationModel.IResourceWithEndpoints { throw null; }
+
+        public static ApplicationModel.IResourceBuilder<TResource> WithHttpCommand<TResource>(this ApplicationModel.IResourceBuilder<TResource> builder, string path, string displayName, string? endpointName = null, System.Net.Http.HttpMethod? method = null, string? httpClientName = null, System.Func<ApplicationModel.HttpCommandRequestContext, System.Threading.Tasks.Task>? configureRequest = null, System.Func<ApplicationModel.HttpCommandResultContext, System.Threading.Tasks.Task<ApplicationModel.ExecuteCommandResult>>? getCommandResult = null, string? commandName = null, System.Func<ApplicationModel.UpdateCommandStateContext, ApplicationModel.ResourceCommandState>? updateState = null, string? displayDescription = null, string? confirmationMessage = null, string? iconName = null, ApplicationModel.IconVariant? iconVariant = null, bool isHighlighted = false)
+            where TResource : ApplicationModel.IResourceWithEndpoints { throw null; }


I'm of the opinion that these APIs (both WithHttpCommand overloads) have way too many parameters. It might be cleaner to encapsulate (or logically group) some of these parameters into a specific DashboardOptions object, or parameter object that group things together to help reduce the number of parameters. @davidfowl

Agree, this looks a little crazy from a versioning POV. We had this problem with WithEndpoint

@DamianEdwards can you do the needful

These expose the same parameters that the WithCommand methods do, plus the few extra specific to HTTP commands. I'm really not sure any other approach is better. Having this API take a new options object that isn't supported by WithCommand doesn't seem to make much sense. Are we thinking we introduce a new type like CommandOptions and use it for both WithCommand and WithHttpCommand, and mark the overloads of WithCommand that don't take it obsolete?

This seems like a lesson that we need to keep learning ;)

Yes, optional parameters at this scale are extremely hard to version. WithEndpoint is in the exact same jail 😄 .

I’d be very happy if we stage a jailbreak on WithEndpoint in 10.0 so we can do proxyless persistent containers by default.

So what's the consensus for 9.2? Change WithCommand and WithHttpCommand to use a new CommandOptions parameter and obsolete the existing WithCommand overload with all the optional parameters?

I'd vote yes, move forward with CommandOptions parameter and obsolete the existing, but that's just me. Also, I found a bug related to this API when testing it: #8270.

Change is in PR #8276

radical · 2025-03-20T22:09:31Z

The tests are not getting triggered on this PR. I found https://github.com/orgs/community/discussions/55906#discussioncomment-5946239

.. and ..
When you use the repository's GITHUB_TOKEN to perform tasks, events triggered by the GITHUB_TOKEN, with the exception of workflow_dispatch and repository_dispatch, will not create a new workflow run.

We are indeed using the token.

aspire/.github/workflows/generate-api-diffs.yml

Lines 27 to 32 in 58cb944

    
           - name: Create or update pull request 
        
             uses: dotnet/actions-create-pull-request@v4 
        
             with: 
        
               token: ${{ secrets.GITHUB_TOKEN }} 
        
               branch: update-api-diffs 
        
               base: main

Maybe we need this which suggests adding a permissions: pull-request: write to the GITHUB_TOKEN, which IIUC should be limited to that workflow.

eerhardt · 2025-03-21T17:46:21Z

src/Aspire.Hosting.Azure/api/Aspire.Hosting.Azure.cs

@@ -120,6 +120,13 @@ public partial interface IAzureResource : IResource

 namespace Aspire.Hosting.Azure
 {
+    public partial class AppliedRoleAssignmentsAnnotation : ApplicationModel.IResourceAnnotation


Does anyone have good options for the name of this annotation? I used this one to start, but I'm not excited about the name.

aspire/src/Aspire.Hosting.Azure/AppliedRoleAssignmentsAnnotation.cs

Lines 8 to 22 in 08b424e

/// <summary>

/// Represents the role assignments that have been applied to an Azure resource

/// and should be included in its Bicep template. These role assignments will grant the

/// <see cref="AzureBicepResource.KnownParameters.PrincipalId"/> permissions to the Azure resource.

/// </summary>

/// <param name="roles">The roles to generate in the resource's Bicep template.</param>

/// <remarks>

/// <see cref="AppliedRoleAssignmentsAnnotation"/> are used when the entire application utilizes a single managed identity.

/// For instance, during local development provisioning, all resources will use the same managed identity,

/// which is the currently logged-in developer.

///

/// When using provisioning infrastructure that supports targeted role assignments (e.g., AddAzureContainerAppsInfrastructure),

/// <see cref="AppliedRoleAssignmentsAnnotation"/> are not used.

/// </remarks>

public class AppliedRoleAssignmentsAnnotation(HashSet<RoleDefinition> roles) : IResourceAnnotation

Other options I've thought of (or copilot suggested that I thought were interesting):

EmbeddedRoleAssignmentsAnnoation

IncludedRoleAssignmentsAnnoation

InlineRoleAssignmentsAnnotation

RoleAssignmentsInBicepAnnotation

RoleAssignmentsInMainBicepAnnotation

ColocatedRoleAssignmentsAnnotation

cc @captainsafia @davidfowl @sebastienros @mitchdenny @ReubenBond

~~Why not RoleAssignmentsAnnotation. Might be missing something.~~

EffectiveRoleAssignmentsAnnotation

EffectiveRoleAssignmentsAnnotation

I'm not sure this is a good name because the role assignments that are external to the resource's bicep are also "effective".

davidfowl · 2025-03-22T04:57:11Z

The tests are not getting triggered on this PR. I found https://github.com/orgs/community/discussions/55906#discussioncomment-5946239

We don't want test triggered on this PR 😄

github-actions bot requested review from radical, eerhardt and sebastienros as code owners February 27, 2025 16:16

davidfowl added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 28, 2025

github-actions bot force-pushed the update-api-diffs branch 7 times, most recently from 55c8b26 to 4bcd5c4 Compare March 7, 2025 16:15

github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 10, 2025

danmoseley added area-engineering-systems infrastructure helix infra engineering repo stuff and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 11, 2025

github-actions bot force-pushed the update-api-diffs branch 5 times, most recently from afdfb7c to 7767997 Compare March 17, 2025 16:17

eerhardt reviewed Mar 17, 2025

View reviewed changes

src/Aspire.Hosting.Kubernetes/api/Aspire.Hosting.Kubernetes.cs Show resolved Hide resolved

github-actions bot force-pushed the update-api-diffs branch from 7767997 to 01dd76c Compare March 20, 2025 03:25