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

Prefer environment variables over local variables #12

Merged
merged 1 commit into from Apr 25, 2022
Merged

Prefer environment variables over local variables #12

merged 1 commit into from Apr 25, 2022

Conversation

5nord
Copy link
Contributor

@5nord 5nord commented Sep 20, 2021

Closes #11

@subosito subosito merged commit cc6b967 into subosito:master Apr 25, 2022
@subosito
Copy link
Owner

Thanks, @5nord!

@luisdavim
Copy link
Collaborator

@subosito @5nord I'm not sure this is entirely correct, it should only prefer environment variables over local variables if overload is not required right? If not, if the user overwrites an existing env var in the file and then on the same file uses that var in the value of another var, the expansion would use the original value set outside insted of the overwriten one...

Maybe variable expansion should only happen after the variables are actually applyed and in that case use only the environment variables instead of the map since in that situation the variables would have already been set as the user intended.

The var expansion could also be done recursively, something like this: https://github.com/Praqma/helmsman/pull/657/files but using the regex instead of os.Expand since that only supports $var and ${var}...

I can open a PR to address this if you agree with what I'm saying.

@5nord
Copy link
Contributor Author

5nord commented May 18, 2022

I want to make sure I understood you correctly. Are you saying expected behaviour as described in issue #11 is not correct?

It seems to me you are describing previous behaviour. What behaviour would you expect? Could you create an example, maybe?

A recursive behaviour would probably deviate from this project's original intention beeing an as close as possible dotenv port, wouldn't it?

@luisdavim
Copy link
Collaborator

luisdavim commented May 18, 2022

What I'm saying is that we have 2 different use cases:

  1. the user wants the env file to overwrite the existing environment variables
  2. the user wants the existing environment variables to take precedence

Before this PR, the variable expansion logic was wrong for the second use case, and now it's wrong for the first one. I think we should support both.

Regarding recursion, think of this scenario:

FOO=BAR
BAR=${FOB}
FOB=${FOO}

If we do the expansion before applying the variables from the file, we have edge cases regarding the order and if the variables referenced in the values are only or also defined outside of the file.
Maybe the best approach would be not to implicitly expand the variables and let the user handle that on their code but we could provide a method to facilitate that.

The recursion is more of a nice to have to eliminate some edge cases but I do think that since the lib allows the users to decide if they want the existing variables to take precedence or not, the variable expansion should take that into consideration.

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.

Unexpected behaviour when referencing variables
3 participants