Reduce the number of env vars exposed to subprocess#1011
Conversation
edmorley
force-pushed
the
cleanup-subprocess-env-vars
branch
from
ba95603 to
0e5b8cc
Compare
|
@Malax I've updated the PR to fix the test runner (by adding |
To prevent external environment variables from leaking into the tests, which otherwise causes problems trying to write tests for #1011. Several tests which were relying on this leak had to be fixed, so that the env vars they were using are set using `ENV_DIR`, as happens in production. Fixes #1014. Fixes #1015.
To prevent external environment variables from leaking into the tests, which otherwise causes problems trying to write tests for #1011. Several tests which were relying on this leak had to be fixed, so that the env vars they were using are set using `ENV_DIR`, as happens in production. Fixes #1014. Fixes #1015.
edmorley
force-pushed
the
cleanup-subprocess-env-vars
branch
from
0e5b8cc to
7de388a
Compare
This fix required several broken tests to be fixed too, so I split those changes out to #1016, and have now rebased on that. |
|
Even after the test harness fixes in #1016 the newly added test here was failing -- and seemingly only on Travis (it passed locally). After two hours of digging into the differences between the way the tests run locally and on Travis (something that's now resolved in #1021), I noticed that actually the failures seen were not Travis-specific but stack-specific, and locally I happened to be using the one stack out of three on which the test passed (Heroku-18) 🤦 The fact that the tests failed on only 2 out of 3 stacks on Travis was hard to spot, since the test matrix was configured to split jobs by test suite and not stack, and Heroku-18 was the last stack to run in each job so the passing test was hidden right at the end of the log. I've now split the stacks out into their own jobs as part of #1022 to prevent this in the future. The test failure itself was due to I managed to create a repro outside the tests, which shows it only affects subshells: Ubuntu 16.04: Ubuntu 18.04: The version of bash in Ubuntu 16.04 is As such I believe it's due to this change: Which is referenced also here: As such we should just exclude that env var from the test on the older stacks. |
edmorley
force-pushed
the
cleanup-subprocess-env-vars
branch
from
7de388a to
9ad20d9
Compare
The following env vars are no longer exposed to subprocesses run by the buildpack (such as the `bin/pre_compile` and `bin/post_compile` hooks): * `BPLOG_PREFIX` * `CACHED_PYTHON_STACK` * `DEFAULT_PYTHON_STACK` * `DEFAULT_PYTHON_VERSION` * `LATEST_27` * `LATEST_34` * `LATEST_35` * `LATEST_36` * `LATEST_37` * `LATEST_38` * `PIP_UPDATE` * `PY27` * `PY34` * `PY35` * `PY36` * `PY37` * `PYPY_27` * `PYPY_36` * `RECOMMENDED_PYTHON_VERSION` * `WARNINGS_LOG` There were previously no tests at all for the pre/post-compile hooks, so I've added some now. Fixes #1010.
edmorley
force-pushed
the
cleanup-subprocess-env-vars
branch
from
9ad20d9 to
bef82ec
Compare
In #1011 the number of buildpack variables that are exported (and so exposed to subprocesses) was reduced, since in general we don't want to leak buildpack internals into end-user steps such as the pre/post compile hooks. However since this change, any buildpack metric emitted from within a `sub_env` wrapper (which is a buildpack-stdlib utility function) is missing its `buildpack.python` prefix. This is because buildpack-stdlib: * lazy-loads `BPLOG_PREFIX` (rather than doing so when initially sourced, which is the approach used for `BUILDPACK_LOG_FILE`) * doesn't check whether `BPLOG_PREFIX` is set before emitting metrics See: https://github.com/heroku/buildpack-stdlib/blob/v8/stdlib.sh As a stop-gap until we either fix this in buildpack-stdlib (W-8095466), or remove usages of the `sub_env` wrapper (since I think they are counter-productive in this buildpack), I've added back the export for `BPLOG_PREFIX`. Fixes @W-8095436@.
In #1011 the number of buildpack variables that are exported (and so exposed to subprocesses) was reduced, since in general we don't want to leak buildpack internals into end-user steps such as the pre/post compile hooks. However since this change, any buildpack metric emitted from within a `sub_env` wrapper (which is a buildpack-stdlib utility function) is missing its `buildpack.python` prefix. This is because buildpack-stdlib: * lazy-loads `BPLOG_PREFIX` (rather than doing so when initially sourced, which is the approach used for `BUILDPACK_LOG_FILE`) * doesn't check whether `BPLOG_PREFIX` is set before emitting metrics See: https://github.com/heroku/buildpack-stdlib/blob/v8/stdlib.sh As a stop-gap until we either fix this in buildpack-stdlib (W-8095466), or remove usages of the `sub_env` wrapper (since I think they are counter-productive in this buildpack), I've added back the export for `BPLOG_PREFIX`. Fixes @W-8095436@.
To prevent external environment variables from leaking into the tests, which otherwise causes problems trying to write tests for heroku#1011. Several tests which were relying on this leak had to be fixed, so that the env vars they were using are set using `ENV_DIR`, as happens in production. Fixes heroku#1014. Fixes heroku#1015.
The following env vars are no longer exposed to subprocesses run by the buildpack (such as the `bin/pre_compile` and `bin/post_compile` hooks): * `BPLOG_PREFIX` * `CACHED_PYTHON_STACK` * `DEFAULT_PYTHON_STACK` * `DEFAULT_PYTHON_VERSION` * `LATEST_27` * `LATEST_34` * `LATEST_35` * `LATEST_36` * `LATEST_37` * `LATEST_38` * `PIP_UPDATE` * `PY27` * `PY34` * `PY35` * `PY36` * `PY37` * `PYPY_27` * `PYPY_36` * `RECOMMENDED_PYTHON_VERSION` * `WARNINGS_LOG` There were previously no tests at all for the pre/post-compile hooks, so I've added some now. Fixes heroku#1010.
In heroku#1011 the number of buildpack variables that are exported (and so exposed to subprocesses) was reduced, since in general we don't want to leak buildpack internals into end-user steps such as the pre/post compile hooks. However since this change, any buildpack metric emitted from within a `sub_env` wrapper (which is a buildpack-stdlib utility function) is missing its `buildpack.python` prefix. This is because buildpack-stdlib: * lazy-loads `BPLOG_PREFIX` (rather than doing so when initially sourced, which is the approach used for `BUILDPACK_LOG_FILE`) * doesn't check whether `BPLOG_PREFIX` is set before emitting metrics See: https://github.com/heroku/buildpack-stdlib/blob/v8/stdlib.sh As a stop-gap until we either fix this in buildpack-stdlib (W-8095466), or remove usages of the `sub_env` wrapper (since I think they are counter-productive in this buildpack), I've added back the export for `BPLOG_PREFIX`. Fixes @W-8095436@.
2 participants
The following env vars are no longer exposed to subprocesses run by the buildpack (such as the
bin/pre_compileandbin/post_compilehooks):BPLOG_PREFIXCACHED_PYTHON_STACKDEFAULT_PYTHON_STACKDEFAULT_PYTHON_VERSIONLATEST_27LATEST_34LATEST_35LATEST_36LATEST_37LATEST_38PIP_UPDATEPY27PY34PY35PY36PY37PYPY_27PYPY_36RECOMMENDED_PYTHON_VERSIONWARNINGS_LOGThere were previously no tests at all for the pre/post-compile hooks, so I've added some now.
Fixes #1010.