rebar3 icon indicating copy to clipboard operation
rebar3 copied to clipboard

Support sys.config.src in common test

Open rkallos opened this issue 3 years ago • 1 comments

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:

  1. Would it be useful for rebar_file_utils:consult_config/2 read the extension of the file, and automatically call consult_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.
  2. Please note, I changed the logic slightly in consult_config to only prepend RootDir if a config file consulted 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.
  3. Should I go ahead and implement this functionality for eunit tests? The way forward appears to be to accept an env_file option in eunit_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 :)
  4. rebar_prv_common_test currently calls rebar_prv_shell:maybe_set_env_vars, introducing a dependency edge between two provider modules that didn't exist before. I don't mind moving maybe_set_env_vars to a module like rebar_file_utils if you'd like to avoid coupling the rebar_prv_shell and rebar_prv_common_test.

rkallos avatar May 28 '21 03:05 rkallos

  1. 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
  2. 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
  3. 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.
  4. 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.

ferd avatar May 28 '21 15:05 ferd