microsoft / microsoft/aspire

Follow-up: address JamesNK review feedback from PR #17756 (Foundry cross-compute-environment references)

Open
#17,771 1 comment 0 reactions 0 assignees View on GitHub
area-integrations triage:bot-seen
Dominant language
C#
Stars
6.3k
Forks
991
Avg merge
2d 15h
Merged PRs (30d)
196

Description

Consolidated follow-up for non-blocking review feedback left by @JamesNK on #17756. These were deferred to keep that PR focused; tracking them here.

### 1. Public API / design durability of the endpoint resolver
**File:** `src/Shared/ComputeEnvironmentEndpointResolver.cs` (~line 23)
> Will everyone building hosting/deploy integrations require code like this? Would this be made public sometime in the future? I want to make sure the design holds up beyond our own bits.

Decide whether cross-compute-environment endpoint resolution should become a public, supported extensibility point for third-party hosting/deploy integrations rather than living in `src/Shared`. Validate the design holds up beyond first-party usage.

[Thread](https://github.com/microsoft/aspire/pull/17756#discussion_r3331227779)

### 2. Add `DebuggerDisplay` to `ReferenceRoleAssignmentAnnotation`
**File:** `src/Aspire.Hosting.Azure/ReferenceRoleAssignmentAnnotation.cs` (~line 32)
> nit: DebuggerDisplay. Most annotations have a DebuggerDisplay. Add one? (might not be true for annotations in integrations/hosting. Could be fixed up in the future.)

Add a `[DebuggerDisplay]` to the annotation for consistency with other annotations.

[Thread](https://github.com/microsoft/aspire/pull/17756#discussion_r3331230624)

### 3. Confirm ordering doesn't matter for `RoleDefinition` set semantics + add tests
**File:** `src/Aspire.Hosting.Azure/ReferenceRoleAssignmentAnnotation.cs` (~line 42)
> Double checking that order will never matter. RoleDefinition is a record so it should override methods needed for working well in a hash set. Are there tests that exercise this?

Confirm `RoleDefinition` record equality/hashing behaves correctly in a `HashSet`, that ordering never affects results, and add test coverage exercising the set semantics.

[Thread](https://github.com/microsoft/aspire/pull/17756#discussion_r3331234819)

### 4. Clarify the `"Azure AI User"` role string
**File:** `src/Aspire.Hosting.Foundry/HostedAgent/HostedAgentBuilderExtension.cs` (~line 446)
> Does this string have to be "Azure AI User", or is it our description? A comment confirming/denying would be useful.

Add a comment clarifying whether `"Azure AI User"` is a required Azure built-in role name or our own description.

[Thread](https://github.com/microsoft/aspire/pull/17756#discussion_r3331236120)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.