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

Add option in Config to not remove prefix when expanding default values which resolve to other environment variables #103

Closed
wants to merge 2 commits into from

Conversation

mlevieux
Copy link

@mlevieux mlevieux commented Feb 27, 2024

Hi there!

The latest v1.0.0 version includes breaking changes, among which the fact that a prefix will not be taken into account when expanding a default value that resolves to another environment variable.

I get the rationale, but find it unfortunate that this was a breaking change and the now default behaviour, as it now makes some code very difficult to reuse and inelegant. Of course, it is possible to implement one's own Lookuper in place of using those provided by the library, but in the case of highly dynamic configurations, that also makes for convoluted code.

I suggest making that optional, so that:

  1. we do not introduce yet another breaking change, and we don't need to wait for 2.0.0 to merge this (the change is backwards compatible);
  2. anyone can choose to profit from this library with or without prefix removal on expansion, or even instantiate Config according to the use-case;

This is what this PR implements. Also, I took advantage of this PR to fix a few typos here and there in the doc comments.

Tell me what you think! 😄

@sethvargo
Copy link
Owner

Hi @mlevieux - it was an intentional decision to break this API in v1.0.0, since the previous behavior was broken and should not have existed. As you mentioned, you can use a custom lookuper to restore the old behavior. That's arguably better, because it makes the decision explicit.

I'm happy to review a PR that fixes typos (although "different than" is not a typo).

@sethvargo sethvargo closed this Feb 27, 2024
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.

2 participants