decrease optional var logging level and allow to pass extra opts to pavics-compose.sh up -d
#426
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
…DEBUG To avoid drowing real WARN messages. Many optional vars can be valid if empty.
github-actions
bot
added
ci/deployment
fmigneault
approved these changes
Feb 23, 2024
When disabling components, their existing containers will not be removed unless option `--remove-orphans` is given together with `./pavics-compose.sh up -d`. This change allow any additional options, not just `--remove-orphans`.
tlvu
changed the title
pavics-compose.sh up -d
|
FYI I added another very small change. PR title and description updated. |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2498/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-optional-vars-log-level 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/1532/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2499/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-optional-vars-log-level DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-133.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1533/NOTEBOOK TEST RESULTS |
mishaschwartz
approved these changes
Feb 24, 2024
CHANGES.md
Outdated
|
|
||
| - logging: decrease logging level for empty optional vars from WARN to DEBUG | ||
|
|
||
| To avoid drowing real WARN messages. Many optional vars can be valid if empty. |
birdhouse/env.local.example
Outdated
| @@ -189,6 +189,10 @@ export GEOSERVER_ADMIN_PASSWORD="${__DEFAULT__GEOSERVER_ADMIN_PASSWORD}" | |||
| # within the autodeploy docker container as the root user. | |||
| #export AUTODEPLOY_CODE_OWNERSHIP="1000:1000" | |||
|
|
|||
| # Extra options for 'pavics-compose.sh up -d'. | |||
There was a problem hiding this comment.
This is a nitpick, but it should be 'pavics-compose.sh up' since -d itself is an extra optional flag
There was a problem hiding this comment.
Agreed ! Too used to always use up with -d that I just type it without thinking about it. In fact, I think for my env.local, I will bake -d in that new var to ensure I never forget to use -d !
If you forget to use -d, it goes to the foreground, and the only way out is Ctrl-C which will stop the entire stack, so annoying.
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2501/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-optional-vars-log-level 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/1535/NOTEBOOK TEST RESULTS |
fmigneault
approved these changes
Feb 26, 2024
| @@ -116,7 +119,7 @@ if [ x"$1" = x"up" ]; then | |||
| fi | |||
|
|
|||
| # the PROXY_SECURE_PORT is a little trick to make the compose file invalid without the usage of this wrapper script | |||
| PROXY_SECURE_PORT=443 HOSTNAME=${PAVICS_FQDN} docker-compose ${COMPOSE_CONF_LIST} $* | |||
There was a problem hiding this comment.
Should there be some kind of duplicate filtering, or we simply let the command fail with any (possibility ambiguous) error message?
For example, COMPOSE_EXTRA_OPTS="-d" and the user does pavics-compose up -d by mistake.
There was a problem hiding this comment.
Good catch, yes double -d do not work ! Weird ! Which reason would motivate such a strict cli args parsing !
There was a problem hiding this comment.
Often CLI libraries support duplicating flags to increase some level. For example, -v means verbose level 1 and -v -v means verbose level 2.
Differentiating between options that support this duplication and those that don't are important to these argument parsing libraries.
There was a problem hiding this comment.
Right ! But activating this "strict count" for all options seems excessive !
mishaschwartz
requested changes
Feb 26, 2024
There was a problem hiding this comment.
Given this point (#426 (comment)) I think that the COMPOSE_EXTRA_OPTS has the potential to cause a lot of confusion.
I think that a better strategy is to encourage users to export any of the environment variables from
- https://docs.docker.com/compose/environment-variables/envvars/
- https://docs.docker.com/engine/reference/commandline/cli/#environment-variables
in their env.local file instead.
For example, instead of setting:
export COMPOSE_UP_EXTRA_OPTS="--remove-orphans"
they would set:
export COMPOSE_REMOVE_ORPHANS=true
Not all CLI options/flags are available as environment variables but I think that enough are that we can cover at least the basic use-cases and this is a well documented feature of docker compose.
Also, the precedence is clearly documented here: https://docs.docker.com/compose/environment-variables/envvars-precedence/ so we can avoid some confusion.
True ! If docker-compose already have some built-in env var to solve my problem, why re-invent the wheel. It does not cover all the options yet, but it's probably enough for now. I'll just document this in |
This reverts commit 7567e8d. Docker-compose has built-in `COMPOSE_REMOVE_ORPHANS=true` to achieve the same thing.
To remove orphans containers when components are disabled. Also link to full documentations if other env var can be used.
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2505/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-optional-vars-log-level 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/1539/NOTEBOOK TEST RESULTS |
mishaschwartz
approved these changes
Feb 27, 2024
|
Saddly, I tested and So how about having |
tlvu
changed the title
pavics-compose.sh up -d
I don't think it needs to be hidden, but we need a better way of telling the user what the cause of an error is. Maybe something like: |
|
@mishaschwartz's proposal seems fine to me. As long as there's a way to figure out from the logs why the command could have failed. |
Great, so I will un-backout my previous commit :) Luckily I did not rush to merge this "simple" change on Friday. |
…peration"" This reverts commit dead9ab. Because `COMPOSE_REMOVE_ORPHANS` do not work.
Before, all the `post-docker-compose-up` would still execute after `docker-compose` has an error.
tlvu
changed the title
pavics-compose.sh up -d
mishaschwartz
requested changes
Feb 28, 2024
…dback) Otherwise we have `docker-compose --remove-orphans up -d`.
mishaschwartz
requested changes
Feb 28, 2024
birdhouse/pavics-compose.sh
Outdated
| @@ -120,7 +120,7 @@ fi | |||
|
|
|||
| log INFO "Executing docker-compose with extra options: ${COMPOSE_EXTRA_OPTS} $*" | |||
There was a problem hiding this comment.
And we should switch the log message to match too
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2507/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-optional-vars-log-level 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/1541/NOTEBOOK TEST RESULTS |
…aschwartz feedback)
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2508/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-optional-vars-log-level DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-133.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1542/NOTEBOOK TEST RESULTS |
mishaschwartz
approved these changes
Feb 28, 2024
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2509/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-optional-vars-log-level 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/1543/NOTEBOOK TEST RESULTS |
fmigneault
approved these changes
Feb 28, 2024
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2521/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-optional-vars-log-level 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/1546/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2522/Result : failure BIRDHOUSE_DEPLOY_BRANCH : fix-optional-vars-log-level DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-133.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1547/NOTEBOOK TEST RESULTS |
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.
logging: decrease logging level for empty optional vars from WARN to DEBUG
To avoid drowning real WARN messages. Many optional vars can be valid if empty.
config: add sample config to configure docker-compose to remove orphans
To remove orphans containers when components are disabled. Also link to full
documentations if other env var can be used.
compose script: allow to pass extra options to
upoperationThe previous docker-compose built-in env var was not working so had to add
this homegrown solution.
When disabling components, their existing containers will not be removed
unless option
--remove-orphansis given together with./pavics-compose.sh up -d.This change allow any additional options, not just
--remove-orphans.compose script: exit early when any errors occurred during invocation
Before, all the
post-docker-compose-upwould still execute afterdocker-composehas an error.