Skip to content

Conversation

@WhitWaldo
Copy link
Contributor

@WhitWaldo WhitWaldo commented Aug 14, 2024

Description

I've made a couple of changes here:

  1. Refactored DaprDefaults to pull the environment variable names out to public constants so other classes can reference them (and eliminate possible typos). Also refactored the default ports for the same reason.
  2. Updated the DaprServiceCollectionExtensions to prefer pulling the Dapr HTTP endpoint/port, gRPC endpoint/port and API token from IConfiguration, failing over to pull from their respective environment variables directly and then defaulting to their default localhost ports if not otherwise available.

I noticed while putting this together that the environment variables documentation doesn't include either DAPR_HTTP_ENDPOINT nor DAPR_GRPC_ENDPOINT and have created a separate PR to include them.

Possible breaking change

But as neither DAPR_HTTP_ENDPOINT nor DAPR_GRPC_ENDPOINT are otherwise indicated in the docs to clarify this, I assume it's a bug that today's DaprDefaults will ignore the ports specified in DAPR_HTTP_PORT or DAPR_GRPC_PORT if the endpoint is specified (opting to use the endpoint specified with its port instead) and will only use them when the endpoint isn't specified (and defaults instead to "127.0.0.1").

I don't think that's necessarily right - that the endpoint should generally specify only the schema and hostname and that if the port is provided, it should be used (instead of potentially inferring the port from the schema, e.g. https => 443 or http => 80). As such, this implementation changes this behavior and will set the endpoint (schema, host and port) and then set the port if also specified.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1336

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

This isn't yet ready to merge as I want to apply the same to the other Dapr clients as well (Jobs, Workflow, etc.) and largely get away from using DaprDefaults where possible.

Also, this presently builds atop the Jobs PR, so it appears a little heavier than it necessarily is.

@WhitWaldo
Copy link
Contributor Author

Closing - going to replace with a PR not based on the Jobs PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prioritize IConfiguration for environment variable retrieval during DI registrations

1 participant