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

Support sys.config.src in common test #2571

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rkallos
Copy link

@rkallos rkallos commented May 28, 2021

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.

@ferd
Copy link
Collaborator

ferd commented May 28, 2021

  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.

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.

Support sys.config.src in ct
2 participants