dnsproviders: Implement NewConfigFromEnv() #1058
Conversation
This allows getting a config populated from env variables. Without this, the only way to do this is to copy/duplicate all the env var logic. NewConfigFromEnv() treats env variables as optional defaults. Since the config struct is returned, its values can still be set/changed/read by the caller, which is immensely useful during application setup/config. See go-acme#1054
|
You have to allow maintainers to modify your PR. |
|
What do you want to change? |
|
The behavior is not good: |
|
Hmm, why is that? I do not think this function should return an error. Env vars aren't required since the config is being returned so the caller can further populate its fields. If we want to verify a config and return errors, that should be done in |
|
Do you have run the tests? This function must return errors. |
|
I understand what you want to do but it is not compatible with the behavior that I want. So we need to find another solution. |
|
What behavior do you want, exactly? |
|
the tests fail on my computer, it's not related to the CI. |
|
Ah, I found it. (I've been having major issues with VS Code + gopls and modules lately for some reason.) One moment, will submit a corrective commit for the test. |
|
Don't fix the tests for now. I repeat I don't agree with your changes. |
|
@ldez Too late 😄 These tests assume that env variables are required, which is not the case since the struct can be filled out by the caller. |
|
You cannot remove the tests. Also I don't agree with that. |
Well, I did remove the tests, because with this refactor they are no longer relevant. The remaining tests cover NewConfigFromEnv because NewDNSProvider calls it directly. Er, what don't you agree with, exactly? |
|
I think we did not understand each other in the issue, we do not have the same vision of the behavior of the method. Now I see what do you want, and I'm reconsidering your request. |
|
I cannot accept your PR, the error from There are several thing that can produce those errors (not only missing env vars) and message details (in the missing env var case) are important. I refuse to change the behavior only for another application (even if it's Traefik or Caddy). As solution, I think we can expose the env vars names to help you. So for now, I will close this PR and continue to discuss on the issue #1054. |
This allows getting a config populated from env variables.
Without this, the only way to do this is to copy/duplicate all the env var logic.
NewConfigFromEnv()treats env variables as optional defaults.Since the config struct is returned, its values can still be set/changed/read by the caller, which is immensely useful during application setup/config.
In practice, this allows us to keep credentials out of config files, while still being able to configure other elements of the DNS provider in code without relying on global env vars (which we can't do in Caddy 2, since we don't really have much/any global state -- so using a global value would lead to incorrect results).
Fixes #1054
I am just starting out with a few providers of my choice, but I am sure there will be more to follow!