Skip to content

Feature/replace os vars test#527

Merged
lrascao merged 1 commit intoerlware:masterfrom
lrascao:feature/replace_os_vars_test
Oct 27, 2016
Merged

Feature/replace os vars test#527
lrascao merged 1 commit intoerlware:masterfrom
lrascao:feature/replace_os_vars_test

Conversation

@lrascao
Copy link
Copy Markdown
Collaborator

@lrascao lrascao commented Oct 23, 2016

The first run would correctly replace the environment variables, however it would also overwrite the original vm.args and sys.config thus preventing any further substitution in subsequent runs. Dev mode runs were also broken, all runs after the first were required to also define the RELX_REPLACE_OS_VARS variable in order not to overwrite the current vm.args with the original one,
this prevented simply attaching to an already running node that was started this way.

This involved reverting part of #441 mostly to simplify the logic
which is now: sys.config and vm.args are created both in dev and normal mode, if os vars replace is requested then a copy of these files is made once and kept in sys.config.orig and vm.args.orig, subsequent runs use these original files to continue performing the variable substitution. If a run is made without the RELX_REPLACE_OS_VARS set any existent .orig files are renamed to their original names.

@lrascao lrascao force-pushed the feature/replace_os_vars_test branch 9 times, most recently from 938a6fb to 3595929 Compare October 24, 2016 17:09
# the result is saved to vm.args
awk '{while(match($0,"[$]{[^}]*}")) {var=substr($0,RSTART+2,RLENGTH -3);gsub("[$]{"var"}",ENVIRON[var])}}1' < "$orig_vmargs_path" > "$VMARGS_PATH"
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What will happen if you run the second time without RELX_REPLACE_OS_VARS? Would you want to run it from modified vm.args or original vm.args?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah, i had some doubts about this behaviour, i ended up going with run from modified vm.args, the reason being that the original probably has env variables that won't get replaced and most likely will cause the release start to fail, what are your thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be simpler if we just maintain the following invariant:
if RELX_REPLACE_OS_VARS then modify else use original.
irrespective of whether it is first, second .... run

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ok, i think that was the previous behaviour, let's keep that way then.
I've updated the PR with that change and the tests, please check it when time is available

@lrascao lrascao mentioned this pull request Oct 24, 2016
@lrascao lrascao force-pushed the feature/replace_os_vars_test branch from 3595929 to 18cf500 Compare October 24, 2016 21:55
@lrascao lrascao force-pushed the feature/replace_os_vars_test branch 2 times, most recently from ca620fe to d3ac144 Compare October 24, 2016 23:43
Copy link
Copy Markdown
Contributor

@GoelDeepak GoelDeepak left a comment

Choose a reason for hiding this comment

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

lgtm

@lrascao lrascao force-pushed the feature/replace_os_vars_test branch from d3ac144 to 11d22ee Compare October 25, 2016 11:04
@lrascao
Copy link
Copy Markdown
Collaborator Author

lrascao commented Oct 25, 2016

@tsloughter any comments on this? if not i'll go ahead and merge it to get common ground for the other pending PRs related with this functionality

@sargun
Copy link
Copy Markdown

sargun commented Oct 26, 2016

This also cleans up the init script a little bit, which is nice. 👍

The first run would correctly replace the environment
variables, however it would also overwrite the original
vm.args and sys.config thus preventing any further
substitution in subsequent runs.
Dev mode runs were also broken, all runs after the
first were required to also define the
RELX_REPLACE_OS_VARS variable in order not to
overwrite the current vm.args with the original one,
this prevented simply attaching to an already running
node that was started this way.

Add tests to exercise this functionality.
@lrascao lrascao force-pushed the feature/replace_os_vars_test branch from 11d22ee to f0e0d1a Compare October 27, 2016 17:58
@lrascao lrascao merged commit 3ecc5c8 into erlware:master Oct 27, 2016
@lrascao lrascao deleted the feature/replace_os_vars_test branch October 27, 2016 18:07
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.

3 participants