Skip to content

Remove duplicate port check and use endpoint configured builder #9176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

captainsafia
Copy link
Member

Fixes #8589.

Validated in VS 17.14 Preview 6.

This pull request refactors the WithFunctionsHttp method in AzureFunctionsProjectResourceExtensions.cs to simplify conditional logic and improve readability. The changes primarily focus on consolidating conditional expressions and restructuring the handling of the port parameter.

Refactoring and Simplification:

  • Consolidated the conditional logic for setting up HTTP/HTTPS endpoints by using a ternary operator to assign the appropriate WithHttpEndpoint or WithHttpsEndpoint method to the builder variable. This reduces code duplication and improves readability. ([src/Aspire.Hosting.Azure.Functions/AzureFunctionsProjectResourceExtensions.csL166-R168](https://github.com/dotnet/aspire/pull/9176/files#diff-7739766f9496b7f887e020f33717d1a0402d2003a93a6b561ce86304e2fa9170L166-R168))

  • Restructured the logic for handling the port parameter by removing redundant checks and separating the port == null condition into its own block. This ensures clarity and avoids unnecessary nested conditions. ([src/Aspire.Hosting.Azure.Functions/AzureFunctionsProjectResourceExtensions.csL183-R188](https://github.com/dotnet/aspire/pull/9176/files#diff-7739766f9496b7f887e020f33717d1a0402d2003a93a6b561ce86304e2fa9170L183-R188))

@captainsafia captainsafia requested review from Copilot and phenning May 8, 2025 19:02
@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label May 8, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors the Azure Functions HTTP configuration in the resource extensions to remove duplicate port checking and streamline endpoint configuration.

  • Replaces an if/else block with a ternary operator for endpoint assignment.
  • Splits the logic handling the port parameter by isolating the port-null check.
Comments suppressed due to low confidence (1)

src/Aspire.Hosting.Azure.Functions/AzureFunctionsProjectResourceExtensions.cs:166

  • [nitpick] Ensure consistency in using the ternary operator in similar endpoint configurations across the codebase to maintain a uniform coding style.
builder = useHttps ? builder.WithHttpsEndpoint(port: port, targetPort: port, isProxied: port == null) : builder.WithHttpEndpoint(port: port, targetPort: port, isProxied: port == null);

var targetEndpoint = builder.Resource.GetEndpoint(useHttps ? "https" : "http");
context.Args.Add("--port");
context.Args.Add(targetEndpoint.Property(EndpointProperty.TargetPort));

Copy link
Preview

Copilot AI May 8, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider adding a comment that explains why the port configuration logic is only applied when port is null, to improve clarity regarding the intent of this separation.

Suggested change
// If the port is null, it means the launch profile did not explicitly define a port.
// In this case, we pass the --port argument to the Azure Functions host to ensure
// it listens on the correct port. This avoids conflicts with ports already defined
// in the launch profile and ensures proper configuration during local development.

Copilot uses AI. Check for mistakes.

@phenning
Copy link
Contributor

phenning commented May 8, 2025

It's been a bit since I looked at this, but as I recall when I was debugging though this, I thought that failure to pass the launchSettings defined port back to VS as context args was causing us to launch functions with the default port.

Does this work with the current VS as expected?

@mitchdenny
Copy link
Member

(just check what @phenning mentioned)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
3 participants