Fix DELAYED_EVAL not working for other than pavics compose.sh
#288
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Improved read_configs: * password hiding when reading env.local (caught some password leak) * ensure current dir is COMPOSE_DIR when reading components default.env * new read_basic_config_only() for use by autodeploy
read-configs improvements: * Warn when default.env and env.local not found so we can confirm value of COMPOSE_DIR used.
Was like that before, feature was lost when converted to use read-configs.include.sh.
…ide of 'pavics-compose.sh'
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1206/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-92.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL :NOTEBOOK TEST RESULTS
|
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1207/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-92.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL :NOTEBOOK TEST RESULTS
|
matprov
reviewed
Feb 8, 2023
fmigneault
requested changes
Feb 8, 2023
birdhouse/read-configs.include.sh
Outdated
Comment on lines
18
to
37
| if [ -e "./pavics-compose.sh" ]; then | ||
| # Current dir is COMPOSE_DIR | ||
| COMPOSE_DIR="`realpath .`" | ||
| elif [ -e "../pavics-compose.sh" ]; then | ||
| # Parent dir is COMPOSE_DIR | ||
| # Case of all the scripts under deployment/ or scripts/ | ||
| COMPOSE_DIR="`realpath ..`" | ||
| elif [ -e "../birdhouse-deploy/birdhouse/pavics-compose.sh" ]; then | ||
| # Case of sibling checkout at same level as birdhouse-deploy. | ||
| COMPOSE_DIR="`realpath "../birdhouse-deploy/birdhouse"`" | ||
| elif [ -e "../../birdhouse-deploy/birdhouse/pavics-compose.sh" ]; then | ||
| # Case of subdir of sibling checkout at same level as birdhouse-deploy. | ||
| COMPOSE_DIR="`realpath "../../birdhouse-deploy/birdhouse"`" | ||
| elif [ -e "../../../birdhouse-deploy/birdhouse/pavics-compose.sh" ]; then | ||
| # Case of sub-subdir of sibling checkout at same level as birdhouse-deploy. | ||
| COMPOSE_DIR="`realpath "../../../birdhouse-deploy/birdhouse"`" | ||
| elif [ -e "./birdhouse/pavics-compose.sh" ]; then | ||
| # Child dir is COMPOSE_DIR | ||
| COMPOSE_DIR="`realpath birdhouse`" | ||
| fi |
There was a problem hiding this comment.
For readability, a for loop seems a better to try/break various options.
Also, is there a technical (older shell support?) reason to use `` instead of $()? Using a shell-checker on this file flags those calls. $() is better as it allows nesting calls and are more readable.
There was a problem hiding this comment.
A for-loop to try/break various options, could you give an example? I am not sure how to write a for-loop in this case.
And yes I used backticks instead of $() for portability reason. I try to stay within the limited features of the original Bourne shell (/bin/sh) and not Bourne Again shell (/bin/bash).
Now that you asked, I tried to search for differences between sh and bash and I could not find anything definitive which syntax is which shell. So to play safe, I'd rather stay with backticks for now if you don't mind.
There was a problem hiding this comment.
Something similar to this:
TESTS="
test1
../test2
../../test3
./test4
"
for path in ${TESTS}; do
if [ -d "${path}" ]; then
echo "Found ${path}"
COMPOSE_DIR="${path}"
break
fi
done
There was a problem hiding this comment.
$() are ok in sh
I'm using it (and have to because it's nested) here:
https:/bird-house/birdhouse-deploy/blob/master/birdhouse/scripts/get-components-json.include.sh#L28
There was a problem hiding this comment.
I switched to $() except in process_delayed_eval since I could not get the eval to work properly. Looks like in some corner cases backticks still have its use.
As for the for-loop try/catch, it's not applicable to this case because there is a different mapping for each case:
./pavics-compose.sh -> realpath .
../pavics-compose.sh -> realpath ..
./birdhouse/pavics-compose.sh -> realpath birdhouse
../birdhouse-deploy/birdhouse/pavics-compose.sh -> realpath ../birdhouse-deploy/birdhouse
...
tlvu
changed the title
pavics compose.shDELAYED_EVAL not working for other than pavics compose.sh
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1217/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-36.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/947/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1219/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-36.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/950/NOTEBOOK TEST RESULTS |
|
@eyvorchuk FYI, you are tagged in this PR since you might write custom scripts that might need to read the config files. This PR provided a unified way to read config files. |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1220/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-36.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/951/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1222/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-36.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/953/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1224/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-36.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/955/NOTEBOOK TEST RESULTS |
fmigneault
reviewed
Feb 9, 2023
There was a problem hiding this comment.
Changes are good and seem to work.
Only items in #288 (comment) to consider, but ok to integrate.
… fmigneault I used backticks for portability (restrict to /bin/sh syntax) but no proof that $() is less portable than backticks. According to fmigneault, $() is better as it allows nesting calls and are more readable.
The eval'ed value is wrong:
```
ERROR: Named volume "20919{i}/public-share:/notebook_dir/public:ro"
```
I still need an approval to merge this. |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1225/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-36.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL :NOTEBOOK TEST RESULTS
|
|
https://daccs-jenkins.crim.ca/job/DACCS-iac-birdhouse/1225/console I think it's probably not my change from using backticks to |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1228/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-pavics-compose.sh DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : false PAVICS_HOST : https://host-140-216.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/956/NOTEBOOK TEST RESULTS |
@tlvu Not sure about the reason. I've tried deploying using this branch in build 1227 but it was successful. 1228, manually started with the same params, is working however for unknown reason. One thing I might change in the pipeline is that if the 1200 seconds timeout error occur, instance will not be destroyed by default. Simple, and will avoid having to launch the pipeline twice to diagnose. |
|
@fmigneault Do you see anything blocking? I'd like to merge this PR soon because it is actually fixing a regression. |
|
Have we considered avoiding the need to avoid a export MAGPIE_DB_NAME="magpiedb"
export MAGPIE_PERSIST_DIR='${DATA_PERSIST_ROOT}/magpie_persist'
# and then add MAGPIE_PERSIST_DIR to the DELAYED_EVAL listwe do export MAGPIE_DB_NAME=${MAGPIE_DB_NAME:-"magpiedb"}
export MAGPIE_PERSIST_DIR="${MAGPIE_PERSIST_DIR:-${DATA_PERSIST_ROOT}/magpie_persist}"or if we want to exit early if required variables are not set: export MAGPIE_PERSIST_DIR="${MAGPIE_PERSIST_DIR:-${DATA_PERSIST_ROOT:?DATA_PERSIST_ROOT must be set}/magpie_persist}" || exit 1and then if we source the This will make the |
|
@mishaschwartz The delayed eval trick was needed for
Because With delayed eval, Even without this bug caused by the introduction of delayed eval, it is still a good idea to centralize the configs reading logic to avoid errors when each script does it manually, ex: forgot to hide password when reading |
…t-working-for-other-than-pavics-compose.sh
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1240/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-92.rdext.crim.ca Infrastructure deployment failed. Instance has not been destroyed. @matprov |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1241/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-88.rdext.crim.ca Infrastructure deployment failed. Instance has not been destroyed. @matprov |
|
@tlvu I completely agree that centralizing the config reading functionality is a great idea. I'm sorry I don't think I explained the idea very well. What I was trying to say is that if we source env.local first then we can use parameter expansion in the default.env files (which are sourced afterwards): # env.local
DATA_PERSIST_ROOT=/my/custom/dataroot
# default.env
DATA_PERSIST_ROOT="${DATA_PERSIST_ROOT:-/data}" # /my/custom/dataroot
MAGPIE_PERSIST_DIR="${MAGPIE_PERSIST_DIR:-${DATA_PERSIST_ROOT}/magpie_persist}" # /my/custom/dataroot/magpie_persistI'm not suggesting we change this for this PR and the current method of delaying evaluation definitely works. I'm just suggesting an alternative to think about so that users don't have to worry about knowing whether to single quote or double quote variable values in |
|
One issue I can see with what @mishaschwartz proposes is that multiple configurations of existing instances assume that variables defined in # env.local
MAGPIE_PERSIST_DIR="${DATA_PERSIST_ROOT}/magpie"Which resolves to |
fmigneault
approved these changes
Feb 13, 2023
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1251/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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://host-140-36.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/961/NOTEBOOK TEST RESULTS |
It's not just However it's good to have thinking outside the box like this. Had this trick worked for all cases, it would make config reading much simpler. This trick would work if vars can only reference other vars defined in the same config file, but then there is no way to enforce this. |
github-actions
bot
added
ci/operations
tlvu
deleted the
fix-delayed-eval-not-working-for-other-than-pavics-compose.sh
branch
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1252/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-delayed-eval-not-working-for-other-than-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:// Infrastructure deployment failed. Instance has not been destroyed. @matprov |
This one is normal - branch has been deleted. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
There are other scripts sourcing
default.envandenv.localand all those scripts have to expand the vars inDELAYED_EVALlist to have their actual values.Only scripts using the 3 variables in
DELAYED_EVALlist are broken.DELAYED_EVALwas previously introduced in PR #272.Sample errors
fix-geoserver-data-dir-perm(called at the end ofpavics-compose.sh):trigger-deploy-notebook(broke notebook deploy job):Explanation of the fix
All scripts have to remember to call function
process_delayed_evalin order to obtain the real value of each vars inDELAYED_EVALlist.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 readingenv.local.All scripts should do this going forward