Allow env.local to use delayed eval vars immediately, if needed#644
Allow env.local to use delayed eval vars immediately, if needed#644
env.local to use delayed eval vars immediately, if needed#644Conversation
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/4036/Result ❌ FAILUREBIRDHOUSE_DEPLOY_BRANCH : allow_delayed_eval_usage_in_env.local DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-91.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/652/NOTEBOOK TEST RESULTS |
fmigneault
left a comment
There was a problem hiding this comment.
Might be worth having this quickly mentioned around https://github.com/bird-house/birdhouse-deploy/tree/master/birdhouse#further-explanations to provide the utility function to users that might need it.
OK for me to have the option. Users are in change to deal with the resolution order of things they set in env.local.
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/4037/Result ❌ FAILUREBIRDHOUSE_DEPLOY_BRANCH : allow_delayed_eval_usage_in_env.local DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-154.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/653/NOTEBOOK TEST RESULTS |
mishaschwartz
left a comment
There was a problem hiding this comment.
I like that this adds flexibility to how we can configure the stack.
I left a few comments about the examples you used. I think you could actually simplify the example a lot more to make it clearer what the goal of this is. The example doesn't actually need to show the update to JUPYTERHUB_CONFIG_OVERRIDE because the only place that the JUPYTERHUB_README_FR variable is used is in the if statement.
Just something to think about if you want to make the example more concise and easier to read.
Also please add at least one test for this function in tests/unit/test_read_configs_include.py
| should also be delayed eval'ed. | ||
|
|
||
| ```sh | ||
| export JUPYTERHUB_README_FR="$JUPYTERHUB_USER_DATA_DIR/jupyter-readme/LISMOI.ipynb" |
There was a problem hiding this comment.
Put this in single quotes otherwise this won't wait until the eval_delayed_var is called in order to evaluate JUPYTERHUB_USER_DATA_DIR
There was a problem hiding this comment.
This is real code that we actually use in production and it works as-is.
In env.local it is not necessary to use single-quote because env.local is always last to be read. In all other default.env, you are right, we need single quote because env.local can still override those variables.
There was a problem hiding this comment.
Do you fully override JUPYTERHUB_USER_DATA_DIR=/my/custom/path? In that case, indeed, quotes do not really matter because the result is set as the final value you want anyway.
However, if you used something like
export JUPYTERHUB_USER_DATA_DIR='${BIRDHOUSE_DATA_PERSIST_ROOT}/alternate-jupyter'You could let a test/staging server still set BIRDHOUSE_DATA_PERSIST_ROOT accordingly to different locations, after the fact, with another env file loaded as:
export BIRDHOUSE_EXTRA_CONF_DIRS="
# ... usual/shared stuff ...
/my/deploy-types/staging # load the `default.env` overrides of this specific env
"And you wouldn't have to maintain JUPYTERHUB_USER_DATA_DIR in each sub-env variant.
It is somewhat of a niche use case since you can override anything you want with explicit definitions in the final env.local if you prefer, but still a nice feature.
How about just export JUPYTERHUB_README_FR="$JUPYTERHUB_USER_DATA_DIR/jupyter-readme/LISMOI.ipynb"
# Error, file not found because JUPYTERHUB_README_FR resolve to "${BIRDHOUSE_DATA_PERSIST_ROOT}/jupyterhub_user_data/jupyter-readme/LISMOI.ipynb" because JUPYTERHUB_USER_DATA_DIR is a delayed eval var.
if [ -f "$JUPYTERHUB_README_FR" ]; then
(...)and export JUPYTERHUB_README_FR="$JUPYTERHUB_USER_DATA_DIR/jupyter-readme/LISMOI.ipynb"
eval_delayed_var JUPYTERHUB_README_FR
# Now this works because JUPYTERHUB_README_FR resolve properly to "/data/jupyterhub_user_data/jupyter-readme/LISMOI.ipynb"
if [ -f "$JUPYTERHUB_README_FR" ]; then
(...)
Agreed ! Sorry I didn't meant to use a complex example, that was actual production code that we use. I think the simplification above with additional comments will make it clearer.
There is no need because that new function |
Perfect! That's a much clearer example. thanks
The difference is that we're encouraging users to actually use |
Done deb6c6e |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/4055/Result ❌ FAILUREBIRDHOUSE_DEPLOY_BRANCH : allow_delayed_eval_usage_in_env.local DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-91.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/668/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/4056/Result ❌ FAILUREBIRDHOUSE_DEPLOY_BRANCH : allow_delayed_eval_usage_in_env.local DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-154.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/669/NOTEBOOK TEST RESULTS |
| `JUPYTERHUB_USER_DATA_DIR` is a delayed eval var because it depends on the value of `BIRDHOUSE_DATA_PERSIST_ROOT` which can be | ||
| overridden in `env.local`. So if you need the value of `JUPYTERHUB_USER_DATA_DIR` immediately in `env.local`, you need to | ||
| `eval_delayed_var JUPYTERHUB_USER_DATA_DIR` and if you need to override `BIRDHOUSE_DATA_PERSIST_ROOT`, then you need to override it | ||
| before calling `eval_delayed_var JUPYTERHUB_USER_DATA_DIR`. |
There was a problem hiding this comment.
Can the example be as follows?
Seems quicker to interpret the feature and makes sure the order is explicit.
# in your env.local
export BIRDHOUSE_DATA_PERSIST_ROOT=/my/data # define dependency needed by delayed var
echo "${JUPYTERHUB_USER_DATA_DIR}" # delayed '${BIRDHOUSE_DATA_PERSIST_ROOT}/jupyterhub_user_data'
eval_delayed_var JUPYTERHUB_USER_DATA_DIR # evaluate the desired variable
echo "${JUPYTERHUB_USER_DATA_DIR}" # evaluated '/my/data/jupyterhub_user_data'
Overview
Allow
env.localto use delayed eval var immediatelyBefore,
env.localcan not immediately use any of the delayed eval var because their real values are only available afterenv.localis fully read.So something like this in
env.localdo not work becauseJUPYTERHUB_USER_DATA_DIRis a delayed eval var:env.localcould simply appendJUPYTERHUB_README_FRtoDELAYED_EVALlist but since we need to use its value immediately inenv.local, we need to eval it immediately.With the new
eval_delayed_var, we can do the following, becauseJUPYTERHUB_USER_DATA_DIRis a delayed eval var, any vars that depend on it should also be delayed eval'ed.Changes
Non-breaking changes
eval_delayed_varthatenv.localcan useCI Operations
birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false