Follow-up: address JamesNK review feedback from PR #17756 (Foundry cross-compute-environment references)
- 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
Assessment
This issue has not been assessed yet.