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

fixes for docker compose #198

Merged
merged 1 commit into from
Mar 1, 2023
Merged

fixes for docker compose #198

merged 1 commit into from
Mar 1, 2023

Conversation

ivansenic
Copy link
Contributor

What this PR does:
Fixes docker compose setup:

  • add default value for env JSONTAG
  • removed --wait as it's docker compose v2 option and not all developers have this installed (both @tatu-at-datastax and me can not run it)

@ivansenic ivansenic requested a review from a team as a code owner February 28, 2023 11:25
docker-compose/README.md Show resolved Hide resolved
@@ -37,4 +37,4 @@ export JSONTAG

echo "Running with DSE $DSETAG, Stargate $SGTAG, JSON API $JSONTAG"

docker-compose up -d --wait
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has both its advantages and disadvantages. The advantage is that this can be simply used in any work flow (for it to wait before moving to next step) but the disadvantage is to have the latest docker version.
My take will be to remove '--wait', so we will be compatible with old versions too and also we can work it around in any work flow by adding something like below (this is some thing I added in a Github action post docker compose)

- name: Wait for services
  run: |
     while ! nc -z localhost 8080; do sleep 1; done
     while ! nc -z localhost 8081; do sleep 1; done

Copy link
Contributor

@tatu-at-datastax tatu-at-datastax Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed, FWTW, that Docker desktop had "enable docker compose v2" so at least I was able to upgrade easily. So maybe we will just note this requirement and keep on using --wait.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove it for now, we can add later on if needed and if used in any of the workflows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivansenic ivansenic merged commit 1ac29a3 into main Mar 1, 2023
@ivansenic ivansenic deleted the ise-fix-docker-compose-envs branch March 1, 2023 09:42
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.

3 participants