erlware / relx

Sane, simple release creation for Erlang
http://erlware.github.io/relx
Apache License 2.0
697 stars 232 forks source link

allow to specify default env values in vm.args.src and sys.config.src #759

Closed colrack closed 4 years ago

colrack commented 4 years ago

This patch allows to specify a default value (in a shell like syntax, using :- ) when replacing env vars in vm.args and sys.config eg.

vm.args/vm.args.src

-name ${NODE_NAME:-app@127.0.0.1}

sys.config.src

[{myapp, [{port, ${PORT:-8080}}]}].

Another approach could be allow to specify in relx config a parameter for a file to eval/source in the extended script at runtime. Thoughts?

colrack commented 4 years ago

see #717

tsloughter commented 4 years ago

Nice, I've really wanted this but was always worried a manual implementation would be buggy somehow.

It looks pretty simple but I still want @ferd to check it since he knows how this stuff works and I don't :)

Also, can you add to the tests for this?

ferd commented 4 years ago

Currently though this parsing will fail in the default value for any variable contains :- itself since the split in awk is not stopping on any match:

$ awk 'BEGIN { split("abc",arr,":-"); print arr[1], arr[2], arr[3] }' 
abc  
$ awk 'BEGIN { split("a:-bc",arr,":-"); print arr[1], arr[2], arr[3] }'
a bc 
$ awk 'BEGIN { split("a:-b:-c",arr,":-"); print arr[1], arr[2], arr[3] }'
a b c

Shell substitution rules are different though; the following behaviour is expected in bash, sh, and zsh:

echo "${FAKEVAR:-DEFAULT1:-DEFAULT2}"
DEFAULT1:-DEFAULT2

Right now the patch checks that there are only two matches, but fails when the match is longer than two values. There's no easy way to pull that one off in awk I guess, though there's a possibility that some regex could do it lazily in a way that stops on the first match.

colrack commented 4 years ago

Fred is of course right. I knew about this limitation but I just thought that if anyone is using :- within variables name is going to hurt himself. I can try to fix that, anyway, I would prefer #760 over this. They can of course live together but maybe it is better to have only one way to do things. This one have al least one drawback indeed: inside the VM default variables values would not be defined and I don't think this is good for consistency.

colrack commented 4 years ago

Seems to work even in the case mentioned by Fred now. I will try to add some tests next week and then squash commits.

colrack commented 4 years ago

Test case added, waiting for review

colrack commented 4 years ago

Sorry @lrascao I just notice I didn't actually include the test in the running suite. Going to push again...

lrascao commented 4 years ago

ah cool, didn't notice it as well

colrack commented 4 years ago

OK done, if you want me to squash the last commit just ask

lrascao commented 4 years ago

OK done, if you want me to squash the last commit just ask

yes please