-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix for autodeploy job #176
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just use the already available var COMPOSE_DIR
please.
For testing please test that the autodeploy can fully trigger (ie not just check but actually perform a deployment).
@@ -119,6 +119,8 @@ START_TIME="`date -Isecond`" | |||
echo "========== | |||
triggerdeploy START_TIME=$START_TIME" | |||
|
|||
. ./default.env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$COMPOSE_DIR/default.env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing, i used the daccs-iac, with the birdhouse stack, and I checked the log of the autodeploy cronjob and the job was running without errors. I planned to check also after the merge on a staging setup.
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/546/Result : success BIRDHOUSE_DEPLOY_BRANCH : fix/autodeploy-job DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-4.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/466/NOTEBOOK TEST RESULTS |
Yeah, probably defensively also add a By the way, great PR description, please copy/paste into merge commit description. In fact always do this copy/paste for all your PR, thanks. |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/547/Result : success BIRDHOUSE_DEPLOY_BRANCH : fix/autodeploy-job DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-4.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/467/NOTEBOOK TEST RESULTS |
The new code added with this merge created a new bug for the autodeploy job.
From the autodeploy job's log :
If the
AUTODEPLOY_NOTEBOOK_FREQUENCY
variable is not set in theenv.local
file, it would create the error above.The variable is set in the
default.env
file, in case it is not defined in theenv.local
, and is then used for the new env file from pavics-jupyter-base here.The error happens because the
default.env
was not called in thetriggerdeploy.sh
script, and the variable was not set when running theenv.local
.Solution was tested in a test environment and the cronjob seems to be fixed now.
I tried to see if the same situation could be found anywhere else. From what I see, default.env seems to be called in a consistent way before the env.local.
There is just here, where the default.env doesn't seem to be called. The env.local is called multiple times in that file, but only the first time is the default.env not called.
@tlvu Do you think it could be problematic too? I am not very familiar with that script, how can I verify this?