Skip to content

Add EndpointProperty.HostAndPort - #7629

Merged
Mitch Denny (mitchdenny) merged 6 commits into
microsoft:mainfrom
afscrome:hostandport
Feb 19, 2025
Merged

Add EndpointProperty.HostAndPort#7629
Mitch Denny (mitchdenny) merged 6 commits into
microsoft:mainfrom
afscrome:hostandport

Conversation

@afscrome

@afscromeAlex Crome (afscrome) commented Feb 15, 2025•
edited
Loading

Copy link
Copy Markdown
Collaborator

Description

  • Add EndpointProperty.HostAndPort
  • Update existing places that can now use HostAndPort instead of concatenating Host and Port themselves.

Fixes#7597

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
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

Docs repo has a few places that could be optimised with EndpointProperty.HostAndPort once this is implemented
https://github.com/dotnet/docs-aspire/blob/212a101a3526d6b59823fc8406680f86826dc40a/docs/extensibility/snippets/MailDevResource/MailDev.Hosting/MailDevResource.cs#L27
https://github.com/dotnet/docs-aspire/blob/212a101a3526d6b59823fc8406680f86826dc40a/docs/extensibility/snippets/MailDevResourceAndComponent/MailDev.Hosting/MailDevResource.cs#L27https://github.com/dotnet/docs-aspire/blob/212a101a3526d6b59823fc8406680f86826dc40a/docs/extensibility/custom-hosting-integration.md?plain=1#L360
https://github.com/dotnet/docs-aspire/blob/212a101a3526d6b59823fc8406680f86826dc40a/docs/extensibility/snippets/MailDevResourceWithCredentials/MailDev.Hosting/MailDevResource.cs#L48

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 15, 2025
afscrome
Alex Crome (afscrome) commented Feb 15, 2025
View reviewed changes
Comment threadsrc/Aspire.Hosting/ApplicationModel/EndpointReference.cs Outdated
@afscrome
Add EndpointProperty.HostAndPort
6e217f4
- Add `EndpointProperty.HostAndPort`
- Update existing places that can now use `HostAndPort` instead of concatenating Host and Port themselves.
davidfowl
David Fowler (davidfowl) reviewed Feb 17, 2025
View reviewed changes
Comment threadsrc/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs Outdated
davidfowl
David Fowler (davidfowl) reviewed Feb 17, 2025
View reviewed changes
Comment threadsrc/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs Outdated
davidfowl
David Fowler (davidfowl) reviewed Feb 17, 2025
View reviewed changes
Comment threadsrc/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs Outdated
@davidfowl

David Fowler (davidfowl) commented Feb 17, 2025

Copy link
Copy Markdown
Collaborator

The change is good. I don't see that any tests needed updating so that's a good sign. I think you should look to add an explicit one though.

davidfowl
David Fowler (davidfowl) reviewed Feb 18, 2025
View reviewed changes
Comment threadtests/Aspire.Hosting.Tests/ExpressionResolverTests.cs

Expressions = new()
{
{ "TwoFullEndpoints", ReferenceExpression.Create($"Test1={Endpoint1.Property(EndpointProperty.Scheme)}://{Endpoint1.Property(EndpointProperty.IPV4Host)}:{Endpoint1.Property(EndpointProperty.Port)}/;Test2={Endpoint2.Property(EndpointProperty.Scheme)}://{Endpoint2.Property(EndpointProperty.Host)}:{Endpoint2.Property(EndpointProperty.Port)}/;") },

@davidfowlDavid Fowler (davidfowl)Feb 18, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you add more tests instead of changing these?

@afscromeAlex Crome (afscrome)Feb 18, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Will do - may not be until later this week I'm afraid.

@davidfowlDavid Fowler (davidfowl)Feb 18, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I can merge and you can follow up 😄

@afscromeAlex Crome (afscrome)Feb 18, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Works for me.

@mitchdenny
Mitch Denny (mitchdenny) merged commit ba4dfcf into microsoft:mainFeb 19, 2025
@mitchdenny

Mitch Denny (mitchdenny) commented Feb 19, 2025

Copy link
Copy Markdown
Member

David Fowler (@davidfowl) wait for 9.2 for this one?

@afscromeAlex Crome (afscrome) mentioned this pull request Feb 20, 2025
Closed
@davidfowl

David Fowler (davidfowl) commented Feb 24, 2025

Copy link
Copy Markdown
Collaborator

Yep

@github-actionsgithub-actionsBot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 10, 2025
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 9, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@davidfowlDavid Fowler (davidfowl)davidfowl left review comments
@mitchdennyMitch Denny (mitchdenny)mitchdenny approved these changes

Assignees

No one assigned

Labels

area-app-modelIssues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplicationcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add EndpointProperty.HostAndPort

3 participants

@afscrome@davidfowl@mitchdenny