-
Notifications
You must be signed in to change notification settings - Fork 36
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
fix: use compose v2 syntax #79
base: main
Are you sure you want to change the base?
Conversation
forgot to update the actual compose command, will create a commit for it |
hi @kahnwong Thank you for your contribution to this PR. |
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.
Hi! Thank you for the patch, some nits left.
launch-playground.sh
Outdated
docker-compose down | ||
isComposeV1=`which docker-compose` | ||
if [ $? -eq 0 ]; then | ||
docker-compose up ${components} |
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.
I think we should prioritize docker compose
since it's newer.
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.
I agree with @ryankert01 , since recent version of docker shipped with docker compose
, it's better to use recent version of docker to eliminate potential bugs.
Thank you both for the comments. I agree that it should be
Is there anything I'm missing? |
I propose this version of # 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:
Please let me know if I missed anything. |
Hi @kahnwong, it looks good! Could you commit it to the PR branch? |
ace4325
to
992e72a
Compare
docker compose
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
cc @unknowntpo @xunliu for final review |
LGTM |
Hihi @kahnwong Are you still working on this? This will be QoL improvement for users that use docker compose v2. |
I'm still working on this. I noticed from the latest commit, the file I've worked on has been renamed to another file. Will create a commit for compose syntax upgrade. |
992e72a
to
f4a7879
Compare
@jhchee rebased the pr and apply compose syntax migration. Please kindly review this PR. |
Thanks @kahnwong, I can attest your change is working! However, I'm not a contributor of this project, so will likely need other to approve this PR. |
LGTM, ping @xunliu |
In docker compose v2, the command
docker-compose
has been updated todocker 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 ifdocker 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.