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

Add new 'shell_hooks_env' config to extend shell hooks' OS env vars #2830

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

ziopio
Copy link
Contributor

@ziopio ziopio commented Sep 19, 2023

The option requires a list of Name-Value pairs that are set as env variables when calling the hooks.

{env, [{"MY_VAR", "my-value"}]}.

I tried to add a test in the hooks suite, I do not know if there is a better place.

The option requires a list of Name-Value pairs that are set as env variables when calling the hooks.
Copy link
Collaborator

@ferd ferd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are fine. We need to pick a better name for the config value however.

@@ -58,7 +58,8 @@ create_env(State, Opts) ->
{"ERLANG_TARGET", rebar_api:get_arch()}
],
EInterfaceVars = create_erl_interface_env(),
lists:append([EnvVars, EInterfaceVars]).
ConfigVars = rebar_state:get(State, env, []),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're gonna need a far more descriptive name for this option, especially since rebar_env is called exclusively for hooks right now, and that env can also refer to application or release environment values.

A name like shell_hooks_env would likely be the most descriptive and narrow definition. hooks_env could work but could be confusing for provider hooks rather than shell hooks. env_hooks would fit the overall config pattern but is sort of a stretch as well.

Either way, we need something narrower and more specific than just env here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no problem with shell_hooks_env

@ferd ferd changed the title Add new 'env' key to config options Add new 'shell_hooks_env' config to extend shell hooks' OS env vars Sep 19, 2023
@ferd ferd merged commit f515115 into erlang:main Sep 19, 2023
6 checks passed
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.

2 participants