Skip to content
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

improve docker-compose and fix test ci #138

Closed
wants to merge 11 commits into from
Closed

improve docker-compose and fix test ci #138

wants to merge 11 commits into from

Conversation

pindge
Copy link
Contributor

@pindge pindge commented Dec 2, 2021

  • remove indexer service from docker-compose.yaml
  • add healthcheck to postgres service in docker-compose.yaml
  • remove legacy env setting from test ci
  • improve wps depends_on for postgres readiness
  • remove trailing whitespace from db-setup.sh
  • pin test ci to only run again master branch

@pindge pindge linked an issue Dec 2, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #138 (e634158) into master (bc38b41) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #138   +/-   ##
=======================================
  Coverage   66.25%   66.25%           
=======================================
  Files           8        8           
  Lines         643      643           
=======================================
  Hits          426      426           
  Misses        217      217           

@pindge pindge changed the title remove indexer service from docker-compose improve docker-compose Dec 2, 2021
@pindge pindge requested a review from benjimin December 2, 2021 05:27
@pindge pindge marked this pull request as ready for review December 2, 2021 05:29
@pindge pindge changed the title improve docker-compose improve docker-compose and fix test ci Dec 2, 2021
@pindge pindge linked an issue Dec 2, 2021 that may be closed by this pull request
@benjimin
Copy link
Collaborator

benjimin commented Dec 2, 2021

  • I agree that using healthcheck/dependency correctly is a solution to our main problem, of the runner executing setup-db.sh before the postgres service is ready. However, this solution assumes that docker-compose up -d will not return until all services are started (specifically including the WPS container, which seems a slightly convoluted way to force the runner to wait for the postgres service) and this seems to be an undocumented behaviour. Maybe we should refactor the script to run from inside the WPS container, to guarantee the behaviour?
  • I think test CI should still run against all branches, not just master. (I think its purpose is to help catching bugs before they propagate into the main branch.)
  • The setup-db.sh script will need updating to support removal of the indexer container. (It will periodically be necessary to remove the SQL dump file and let the script regenerate it, due to either new WPS tests or DEA collection reorganisation.)

benjimin added a commit that referenced this pull request Jan 12, 2022
@benjimin
Copy link
Collaborator

Update:

  • Updated setup-db.sh for indexer deprecation c449e6f
  • Applied catalog env var deprecation c100646
  • Ensure setup-db.sh invoked after db ready to accept connections 6c3846d
  • Continuing to let all branches be tested

CI now passing.

Thanks @pindge for all the assistance!

@benjimin benjimin closed this Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

catalog links return 404 Continuous Integration error configuring sample db
2 participants