Skip to content
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

Prioritize retrieval of environment variables from IConfiguration instead of directly #1338

Closed
wants to merge 7 commits into from

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.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…ble names so other classes can reference them.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…ration, then environment variables directly, then default values.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@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