rebar3
rebar3 copied to clipboard
Support sys.config.src in common test
Fixes #2096
Hello!
It's really nice to call rebar3 shell
and have .config.src
files automatically loaded, much like they are in Erlang releases. I was surprised to see that rebar3 ct
doesn't do the same sort of substitution with environment variables. With these changes, it is now possible.
A big portion of this PR is moving consult_env_config/2
from rebar_priv_shell.erl
to rebar_file_utils.erl
.
I have some remaining questions/statements:
- Would it be useful for
rebar_file_utils:consult_config/2
read the extension of the file, and automatically callconsult_env_config
if the extension is.src
? The branching for.config
and.config.src
is currently duplicated, but I'm not exactly sure if it's the right move to DRY in this case. - Please note, I changed the logic slightly in
consult_config
to only prependRootDir
if a config fileconsult
ed from a .config file is a relative path.erl -man config
seems to recommend using absolute paths in sys.config files, so I wanted to be sure that we don't do any unfortunate path mangling when there an absolute path is found. - Should I go ahead and implement this functionality for eunit tests? The way forward appears to be to accept an
env_file
option ineunit_opts
, add a .config.src file to test/rebar_eunit_SUITE_data/syscfg_app.zip, along with an appropriate env_file. I could probably submit that as another PR; just say the word :) -
rebar_prv_common_test
currently callsrebar_prv_shell:maybe_set_env_vars
, introducing a dependency edge between two provider modules that didn't exist before. I don't mind movingmaybe_set_env_vars
to a module likerebar_file_utils
if you'd like to avoid coupling the rebar_prv_shell and rebar_prv_common_test.
- I think we used to do that and it caused all sort of issues with config files we didn't want to be scripts and suddenly started supporting it, like lock files. So no, that causes unexpected dynamism in other places
- We have to somehow support relative paths because the absolute paths don't exist until post deployment, but I'll keep that in mind in the review
- Possibly yeah, we should port it everywhere, but I'll wait to see if the change can work as-is first. I'll need to review this more deeply. I don't think we can this easily change the interface of this call without blowing up a bunch of plugins that do rely on our internals.
- yeah that dependency shouldn't exist. We sort of patch things to work haphazardly to there -- I generally opposed any sys.config dependency within CT tests as bad design (each test could rely on a different config file and the current implementation doesn't work for that) but people keep asking for it so we sort of have to oblige. We'll see how this could be cleaned up.