apache / gravitino-playground

A playground to experience Gravitino
Apache License 2.0
31 stars 31 forks source link

fix: use compose v2 syntax #79

Open kahnwong opened 1 month ago

kahnwong commented 1 month ago

In docker compose v2, the command docker-compose has been updated to docker compose. Ref: https://docs.docker.com/compose/releases/migrate/.

In this pr, in addition to existing check for availability of docker-compose, it also sees if docker compose exists.

Previously, if your system has compose v2, the launch script would fail. With this pr, systems using compose v2 will not result in an exit code.

kahnwong commented 1 month ago

forgot to update the actual compose command, will create a commit for it

xunliu commented 1 month ago

hi @kahnwong Thank you for your contribution to this PR. @unknowntpo Please help review this PR If you have time, Thanks.

kahnwong commented 1 month ago

Thank you both for the comments.

I agree that it should be docker compose syntax, in that case I will do following:

  1. rename this pr to fix: use compose v2 syntax
  2. Checkout code from main/master and replace docker-compose with docker compose

Is there anything I'm missing?

kahnwong commented 1 month ago

I propose this version of launch-playground.sh

# redacted shebang and license

playground_dir="$(dirname "${BASH_SOURCE-$0}")"
playground_dir="$(cd "${playground_dir}">/dev/null; pwd)"

# check for `docker compose`
isExist=`docker --help | grep compose`
if [ $? -eq 0 ]; then
    true # Placeholder, do nothing
else
  echo "ERROR: No docker service environment found, please install compose plugin first."
  exit
fi

components=""
case "${1}" in
  *)
    components=$@
esac

cd ${playground_dir}
docker compose up ${components}

# Clean Docker containers when you quit this script
docker compose down

In which if everyone approves, I will then:

  1. Create a new commit with this version of launch-playground.sh
  2. Rename this pr to: fix: use compose v2 syntax

Please let me know if I missed anything.

ryankert01 commented 1 month ago

Hi @kahnwong, it looks good! Could you commit it to the PR branch?

ryankert01 commented 1 month ago

cc @unknowntpo @xunliu for final review

unknowntpo commented 1 month ago

LGTM