Skip to content

Convert ResourceUrlAnnotation.DisplayOrder from field to property - #13785

Closed
James Newton-King (JamesNK) with Copilot wants to merge 4 commits into
mainfrom
copilot/fix-resourceurlannotation-field
Closed

Convert ResourceUrlAnnotation.DisplayOrder from field to property#13785
James Newton-King (JamesNK) with Copilot wants to merge 4 commits into
mainfrom
copilot/fix-resourceurlannotation-field

Conversation

Copilot AI commented Jan 7, 2026

Copy link
Copy Markdown
Contributor

Description

ResourceUrlAnnotation.DisplayOrder was a public field. Converted to a property with get/set accessors for consistency with other public API members and to follow .NET design guidelines.

Changes

  • ResourceUrlAnnotation.cs: Changed public int? DisplayOrder; to public int? DisplayOrder { get; set; }
  • api/Aspire.Hosting.cs: Updated API baseline to reflect property signature
  • CompatibilitySuppressions.xml: Added CP0008 suppression for field-to-property breaking change

The change is backward compatible for all existing usage patterns (read/write).

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
    • No
Original prompt

This section details on the original issue you should resolve

<issue_title>ResourceUrlAnnotation.DisplayOrder is a field, not a property</issue_title>
<issue_description>https://github.com/dotnet/aspire/blob/17b639949da697093d4320782f24c8190f6f9761/src/Aspire.Hosting/ApplicationModel/ResourceUrlAnnotation.cs#L29-L32</issue_description>

<agent_instructions>Change to a property and add required API compat exemptions.</agent_instructions>

Comments on the Issue (you are Copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
Copilot AI changed the title [WIP] Change ResourceUrlAnnotation.DisplayOrder to a property Convert ResourceUrlAnnotation.DisplayOrder from field to property Jan 7, 2026
@github-actions

github-actions Bot commented Jan 7, 2026

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13785

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13785"

@JamesNK
James Newton-King (JamesNK) marked this pull request as ready for review January 7, 2026 06:58
Copilot AI review requested due to automatic review settings January 7, 2026 06:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR converts ResourceUrlAnnotation.DisplayOrder from a public field to a property with get/set accessors, aligning with .NET design guidelines and ensuring consistency with other members of the class. While the PR description mentions adding a CP0008 suppression to CompatibilitySuppressions.xml, this file is not included in the provided diff.

Key Changes

  • Changed DisplayOrder from a field to a property with auto-implemented getter and setter
  • Updated API baseline file to reflect the new property signature

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Aspire.Hosting/ApplicationModel/ResourceUrlAnnotation.cs Converted DisplayOrder field to property with get/set accessors
src/Aspire.Hosting/api/Aspire.Hosting.cs Updated API baseline to reflect property signature change

Comment thread src/Aspire.Hosting/ApplicationModel/ResourceUrlAnnotation.cs Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
/// The display order of the URL. Higher values mean sort higher in the list.
/// </summary>
public int? DisplayOrder;
public int? DisplayOrder { get; set; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it would be a breaking change. Either we fix it now or never change it.

Do you have suggestions to mitigate? We could put an issue in the CT repo to let them know and document that MCP inspector requires a certain version of Aspire

cc Aaron Powell (@aaronpowell)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to break it I think we should wait for a major version.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. If 13.2 is the next release then this should wait.

@JamesNK James Newton-King (JamesNK) Jan 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have time before this releases, we can help smooth out breaking change by removing usage in community toolkit - CommunityToolkit/Aspire#1088. Then re-add it once CT upgrades to vnext major.

@DamianEdwards Damian Edwards (DamianEdwards) added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jan 8, 2026
@mitchdenny
Mitch Denny (mitchdenny) dismissed their stale review January 13, 2026 22:57

Dismissing own review since consensus is to wait since this is a binary breaking change. That said if we are making other binary breaking changes we should slip this one in at the same time.

@JamesNK James Newton-King (JamesNK) added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 14, 2026
@davidfowl

Copy link
Copy Markdown
Collaborator

Closing as we aren't doing this now.

@dotnet-policy-service dotnet-policy-service Bot added this to the 13.2 milestone Jan 24, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ResourceUrlAnnotation.DisplayOrder is a field, not a property

6 participants