Skip to content

Conversation

Swizzy
Copy link

@Swizzy Swizzy commented Sep 13, 2025

Description

I've added a passthrough parameter to allow me to choose if i want the port to be proxied or not - without this Postgres will use 2 ports when run from Aspire even though it's not really required - i can't override the built-in setup to not be proxied - i can only add an additional port configured to not be proxied. Since this is a relatively minor change i decided to just make a PR with it.

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?

Copy link
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 -- 11384

Or

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

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 13, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 13, 2025
@davidfowl
Copy link
Member

This is a breaking API change, and if we did this it would be another extension method like WithHostPort and then we’d do it everywhere, not just Postgres

@Swizzy
Copy link
Author

Swizzy commented Sep 13, 2025

Could you explain what the breaking change is please? The parameters default value is the same as existing behavior.

@davidfowl
Copy link
Member

I wrote an essay on this in another PR #10062 (comment)

@Swizzy
Copy link
Author

Swizzy commented Sep 13, 2025

Thank you - that makes a lot of sense. I've never had to deal with this kind of situation before and wasn't fully aware of how that works exactly.

Would you say adding a overload with the original signature that simply calls this one would be ok? or should the existing .WithHostPort() extension method be modified to also allow setting this parameter (of course with an overload to maintain compatibility)? perhaps both?

Or would a new extension method named "WithProxiedHostPort" taking the port + bool be a better approach?

I could try to find all other uses as well if you would prefer things to be equal everywhere around this.

@davidfowl
Copy link
Member

Maybe an overload of WithHostPort with 2 parameters the port and isProxied. BTW you can to do this today without any API changes to the resource (look at how WithHostPort is implemented).

The other option is a full control overload: WithHostEndpoint(e => { mutate things here })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants