pavics compose.sh: overriding `DATA_PERSIST_ROOT` in `env.local` do not take effect and warn when a dir in `EXTRA_CONF_DIRS` does not exist #272

tlvu · 2022-12-15T02:52:30Z

Fixes:

Overriding DATA_PERSIST_ROOT in env.local do not take effect for
JUPYTERHUB_USER_DATA_DIR, MAGPIE_PERSIST_DIR, and GEOSERVER_DATA_DIR.

These 3 vars will have to be delayed evaluated for override in env.local to
take effect.

For a variable to be delayed evaluated, it has to be defined using
single-quote and be added to the list of DELAYED_EVAL in default.env.

If those steps are forgotten in env.local, it will still work since
env.local is the last file to be read. However those steps should not be
forgotten in any default.env for all components.

So the impact or burden is on the developpers to write their default.env
file properly, not on the users that only modify the env.local file.

All default.env files header have been updated with notice about this new
delayed evaluation feature.

Fixes 🐛 [BUG]: Regression handling DATA_PERSIST_ROOT setting #270

Changes:

Warn when a dir in EXTRA_CONF_DIRS does not exist.

Most likely a typo in a new dir. Just warn and not exit directly to avoid
leaving the entire platform down during an unattended autodeploy since no
one is around to take immediate action.

Fixes 🐛 [BUG]: Silent error when EXTRA_CONF_DIRS is invalid #266

… components

Do not exit script immediately since this most probably due to a typo in a new conf_dir and exit immediately means the whole platform will be broken. If we just warn and continue, the new conf_dir will not be active but at least all the existing will work. It's not ideal but less bad in the context of unattended autodeploy where no human is available to take corrective action right away.

… exist

crim-jenkins-bot · 2022-12-15T02:53:02Z

E2E Test Results

DACCS-iac Pipeline Results

Build URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1082/
Result : failure

BIRDHOUSE_DEPLOY_BRANCH : fix-bugs-in-pavics-compose.sh
DACCS_CONFIGS_BRANCH : master
PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master
PAVICS_SDI_BRANCH : master

DESTROY_INFRA_ON_EXIT : true
PAVICS_HOST : https://

PAVICS-e2e-workflow-tests Pipeline Results

Tests URL :

NOTEBOOK TEST RESULTS

    
</code>

tlvu · 2022-12-15T02:56:32Z

@fmigneault I changed my mind and go ahead with single-quote and delayed evaluation instead.

The impact on users is actually minimum since forgetting to use single-quote in env.local has no problem. Basically the complexity burden is on the developer only. I've believe I've left enough documentation comments for the developers.

crim-jenkins-bot · 2022-12-15T03:00:00Z

E2E Test Results

DACCS-iac Pipeline Results

Build URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1083/
Result : failure

BIRDHOUSE_DEPLOY_BRANCH : fix-bugs-in-pavics-compose.sh
DACCS_CONFIGS_BRANCH : master
PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master
PAVICS_SDI_BRANCH : master

DESTROY_INFRA_ON_EXIT : true
PAVICS_HOST : https://

PAVICS-e2e-workflow-tests Pipeline Results

Tests URL :

NOTEBOOK TEST RESULTS

    
</code>

aulemahal

AFAIU this looks nice and useful!

birdhouse/default.env

CHANGES.md

birdhouse/pavics-compose.sh

fmigneault · 2022-12-15T23:38:14Z

birdhouse/pavics-compose.sh

+for i in ${DELAYED_EVAL}; do
+  v="`eval "echo \\$${i}"`"
+  eval 'export ${i}="`eval "echo ${v}"`"'
+  echo "delayed eval '`env |grep ${i}=`'"
+done


I'm not sure if I understand the need for the separate DELAYED_EVAL definition.
Wouldn't developers forgetting to specify a new variable name in it be more prone to error?
Couldn't all variables in ${VARS} do the v="`eval "echo \\$${i}"`" step, and this way simply using the single quotes would be sufficient to achieve the same result?

Well, if you think for simplicity, all vars in default.env should be delayed eval then VARS do not contain all of them. VARS currently do not contain the 3 broken vars in DELAYED_EVAL.

Then it looks like delayed eval is an exception rather than the norm. So I'd rather not forced the single quote on more vars than needed.

So they should be defined in OPTIONAL_VARS instead, and this loop should use OPTIONAL_VARS?

It's not clear to me what is your reluctance to have a new var DELAYED_EVAL. Let's say we go with OPTIONAL_VARS, the person changing default.env still have to remember to add any new vars that need delayed eval to OPTIONAL_VARS again.

Basically, if the goal in simplicity, then having to remember to add to DELAYED_EVAL vs OPTIONAL_VARS is the same effort. With DELAYED_EVAL at least the feature is specific and the dev will have to remember to use single-quote.

I am not sure what is the gain to overload OPTIONAL_VARS with a new feature. Currently OPTIONAL_VARS is only used for template substitution. I'd rather have one duty per var for simplicity sake.

I'm not reluctant to it if there's no way around it.
I'm rather trying to see if there's a way we could drop all these manual steps altogether.
Technically, we could generate the contents of OPTIONAL_VARS using only a grep -E 'export [A-Z]=', and then we would not have to worry about those steps ever again.

Ouch, the grep trick might too magic for me. How do you get the name of each var after?

Using this:
cat birdhouse/default.env | grep -E "export [A-Z0-9_]+=" | cut -d ' ' -f 2 | cut -d '=' -f 1

And an extra | sed -r -e 's/(.*)/\$\1/g' at the end for adding the $.

Oh wow, think I'll pass on this magic for now. We have to handle all the default.env from components from any other external repos as well. I don't have time to think of all the corner cases with this magic for now.

Do you see anything blocking with the current approach?

No. Good to go with the current alternative.

…compose.sh

birdhouse/default.env

fmigneault

Current method sufficient for now.
Alternative to avoid DELAYED_EVAL to be considered as some other time as per #272 (comment).

For reference: #274

## Fixes There are other scripts sourcing `default.env` and `env.local` and all those scripts have to expand the vars in `DELAYED_EVAL` list to have their actual values. Only scripts using the 3 variables in `DELAYED_EVAL` list are broken. `DELAYED_EVAL` was previously introduced in PR #272. ### Sample errors `fix-geoserver-data-dir-perm` (called at the end of `pavics-compose.sh`): ``` fix GeoServer data dir permission on first run only, when data dir do not exist yet. + DATA_DIR='${DATA_PERSIST_ROOT}/geoserver' + '[' -n ] + docker run --rm --name fix-geoserver-data-dir-perm --volume '${DATA_PERSIST_ROOT}/geoserver:/datadir' --env FIRST_RUN_ONLY bash:5.1.4 bash -xc 'if [ -z "$FIRST_RUN_ONLY" -o ! -f /datadir/global.xml ]; \ then chown -R 1000:10001 /datadir; else echo "No execute."; fi' docker: Error response from daemon: create ${DATA_PERSIST_ROOT}/geoserver: "${DATA_PERSIST_ROOT}/geoserver" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9_.-]" are allowed. If you intended to pass a host directory, use absolute path. ``` `trigger-deploy-notebook` (broke notebook deploy job): ``` + TMP_SCRIPT=/tmp/notebookdeploy/notebookdeploy.XXXXXXIfafFK/deploy-notebook + cat + chmod a+x /tmp/notebookdeploy/notebookdeploy.XXXXXXIfafFK/deploy-notebook + docker run --rm --name deploy_tutorial_notebooks -u root -v /tmp/notebookdeploy/notebookdeploy.XXXXXXIfafFK/deploy-notebook:/deploy-notebook:ro -v /tmp/notebookdeploy/notebookdeploy.XXXXXXIfafFK/tutorial-notebooks:/tutorial-notebooks:ro -v '${DATA_PERSIST_ROOT}/jupyterhub_user_data:/notebook_dir:rw' --entrypoint /deploy-notebook bash:5.1.4 docker: Error response from daemon: create ${DATA_PERSIST_ROOT}/jupyterhub_user_data: "${DATA_PERSIST_ROOT}/jupyterhub_user_data" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9_.-]" are allowed. If you intended to pass a host directory, use absolute path. ``` ### Explanation of the fix All scripts have to remember to call function `process_delayed_eval` in order to obtain the real value of each vars in `DELAYED_EVAL` list. Centralized all logic about reading configs (config files reading order, remember to call `process_delayed_eval`) to avoid mistake and to ease updating logic in the future. Too many scripts were reading the configs themselves and some are not doing it properly, ex: forgot to hide password when reading `env.local`. ### All scripts should do this going forward ```sh # Set variable COMPOSE_DIR to the dir containing pavics-compose.sh and docker-compose.yml. # Source the script providing function read_configs. # read_configs uses COMPOSE_DIR to find default.env and env.local. . $COMPOSE_DIR/read-configs.include.sh # Call function read_configs to read the various config files in the appropriate order and process delayed eval vars properly. read_configs ```

tlvu added 10 commits December 14, 2022 19:49

pavics-compose: fix DATA_PERSIST_ROOT in env.local do not take effect

01d7fc7

env.local: note to use single-quote for delayed eval

3c99ef5

pavics-compose.sh: improve notice for delayed eval vars

5834219

pavics-compose.sh: wrong location of delayed eval block, not handling…

4a76fae

… components

pavics-compose.sh: confirm delayed eval vars are exported properly

cf4b6ad

pavics-compose.sh: defensive coding with var name

e97ad3b

default.env: add notice to use single-quote for delayed eval

0ddbead

all components: add notice about delayed eval to default.env

de37eab

CHANGES: update for delayed eval and warn when EXTRA_CONF_DIRS do not…

6297120

… exist

tlvu requested review from fmigneault and aulemahal December 15, 2022 02:52

CHANGES: reference fixed issues

0ff509a

tlvu requested a review from matprov December 15, 2022 03:12

aulemahal approved these changes Dec 15, 2022

View reviewed changes

fmigneault reviewed Dec 15, 2022

View reviewed changes

tlvu added 2 commits December 21, 2022 17:28

Merge remote-tracking branch 'origin/master' into fix-bugs-in-pavics-…

cd14d58

…compose.sh

fix wording and typo after PR feedback

489f394

fmigneault reviewed Dec 22, 2022

View reviewed changes

birdhouse/default.env Outdated Show resolved Hide resolved

more wording fix after PR feedback

407c348

tlvu requested a review from fmigneault December 22, 2022 20:02

fmigneault mentioned this pull request Dec 23, 2022

💡 [Feature] Consider automation of the DELAYED_EVAL operation #274

Open

fmigneault approved these changes Dec 23, 2022

View reviewed changes

Bump version: 1.22.6 → 1.22.7

24ac4e1

github-actions bot added ci/operations Continuous Integration components documentation Improvements or additions to documentation labels Dec 23, 2022

tlvu merged commit 2337a16 into master Dec 23, 2022

tlvu deleted the fix-bugs-in-pavics-compose.sh branch December 23, 2022 22:52

tlvu mentioned this pull request Jan 24, 2023

Jupyter: Restore terminal and allow recursive folder deletion #280

Merged

tlvu mentioned this pull request Feb 9, 2023

Fix DELAYED_EVAL not working for other than pavics compose.sh #288

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pavics compose.sh: overriding `DATA_PERSIST_ROOT` in `env.local` do not take effect and warn when a dir in `EXTRA_CONF_DIRS` does not exist #272

pavics compose.sh: overriding `DATA_PERSIST_ROOT` in `env.local` do not take effect and warn when a dir in `EXTRA_CONF_DIRS` does not exist #272

tlvu commented Dec 15, 2022 •

edited

Loading

crim-jenkins-bot commented Dec 15, 2022

tlvu commented Dec 15, 2022

crim-jenkins-bot commented Dec 15, 2022

aulemahal left a comment

fmigneault Dec 15, 2022 •

edited

Loading

tlvu Dec 21, 2022

fmigneault Dec 22, 2022

tlvu Dec 22, 2022

fmigneault Dec 22, 2022 •

edited

Loading

tlvu Dec 22, 2022

fmigneault Dec 22, 2022 •

edited

Loading

fmigneault Dec 22, 2022

tlvu Dec 22, 2022

fmigneault Dec 23, 2022

fmigneault left a comment

pavics compose.sh: overriding DATA_PERSIST_ROOT in env.local do not take effect and warn when a dir in EXTRA_CONF_DIRS does not exist #272

pavics compose.sh: overriding DATA_PERSIST_ROOT in env.local do not take effect and warn when a dir in EXTRA_CONF_DIRS does not exist #272

Conversation

tlvu commented Dec 15, 2022 • edited Loading

Fixes:

Changes:

crim-jenkins-bot commented Dec 15, 2022

E2E Test Results

DACCS-iac Pipeline Results

PAVICS-e2e-workflow-tests Pipeline Results

tlvu commented Dec 15, 2022

crim-jenkins-bot commented Dec 15, 2022

E2E Test Results

DACCS-iac Pipeline Results

PAVICS-e2e-workflow-tests Pipeline Results

aulemahal left a comment

Choose a reason for hiding this comment

fmigneault Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

tlvu Dec 21, 2022

Choose a reason for hiding this comment

fmigneault Dec 22, 2022

Choose a reason for hiding this comment

tlvu Dec 22, 2022

Choose a reason for hiding this comment

fmigneault Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

tlvu Dec 22, 2022

Choose a reason for hiding this comment

fmigneault Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

fmigneault Dec 22, 2022

Choose a reason for hiding this comment

tlvu Dec 22, 2022

Choose a reason for hiding this comment

fmigneault Dec 23, 2022

Choose a reason for hiding this comment

fmigneault left a comment

Choose a reason for hiding this comment

pavics compose.sh: overriding `DATA_PERSIST_ROOT` in `env.local` do not take effect and warn when a dir in `EXTRA_CONF_DIRS` does not exist #272

pavics compose.sh: overriding `DATA_PERSIST_ROOT` in `env.local` do not take effect and warn when a dir in `EXTRA_CONF_DIRS` does not exist #272

tlvu commented Dec 15, 2022 •

edited

Loading

fmigneault Dec 15, 2022 •

edited

Loading

fmigneault Dec 22, 2022 •

edited

Loading

fmigneault Dec 22, 2022 •

edited

Loading