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.

Sign up for GitHub

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

Remove docker compose v1 #2187

Merged
merged 6 commits into from
Jun 9, 2023
Merged

Remove docker compose v1 #2187

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c4d868d
remove docker compose v1
hubertdeng123 Jun 5, 2023
fcbd17f
update all dc_base to use docker compose
hubertdeng123 Jun 5, 2023
3e046e5
use dc v2.0.0 in tests
hubertdeng123 Jun 5, 2023
374abc0
bump min requirement for dc to v2.0.1
hubertdeng123 Jun 5, 2023
88567d1
Merge branch 'master' into hubertdeng123/remove-compose-v1
hubertdeng123 Jun 6, 2023
5ce4fbf
allow symlinking to docker-compose
hubertdeng123 Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/ISSUE_TEMPLATE/problem-report.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ body:
label: Docker Compose Version
placeholder: 2.6.0 ← should look like this (docker compose version)
description: |
What version of docker-compose are you using to run self-hosted?
You need to use docker-compose --version if you are running < v2.0.0.
What version of docker compose are you using to run self-hosted?
validations:
required: true
- type: textarea
Expand Down
5 changes: 0 additions & 5 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,6 @@ jobs:
fail-fast: false
matrix:
include:
# Disabled due to https://github.com/getsentry/self-hosted/issues/1415
# - compose_version: "1.28.0"
# compose_path: "/usr/local/bin"
# - compose_version: "1.29.2"
# compose_path: "/usr/local/bin"
chadwhitacre marked this conversation as resolved.
Show resolved Hide resolved
- compose_version: "v2.0.1"
compose_path: "/usr/local/lib/docker/cli-plugins"
- compose_version: "v2.7.0"
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Official bootstrap for running your own [Sentry](https://sentry.io/) with [Docke
## Requirements

* Docker 19.03.6+
* Compose 1.28.0+
* Compose 2.0.0+
* 4 CPU Cores
* 8 GB RAM
* 20 GB Free Disk Space
Expand Down
2 changes: 1 addition & 1 deletion install/_min-requirements.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Don't forget to update the README and othes docs when you change these!
MIN_DOCKER_VERSION='19.03.6'
MIN_COMPOSE_VERSION='1.28.0'
MIN_COMPOSE_VERSION='2.0.0'
chadwhitacre marked this conversation as resolved.
Show resolved Hide resolved
MIN_RAM_HARD=3800 # MB
MIN_RAM_SOFT=7800 # MB
MIN_CPU_HARD=2
Expand Down
4 changes: 2 additions & 2 deletions install/check-minimum-requirements.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ if [[ "$(vergte ${DOCKER_VERSION//v/} $MIN_DOCKER_VERSION)" -eq 1 ]]; then
fi
echo "Found Docker version $DOCKER_VERSION"

COMPOSE_VERSION=$($dc_base version --short || echo '')
COMPOSE_VERSION=$(docker compose version --short || echo '')
Copy link
Member

Choose a reason for hiding this comment

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

I would expect to see $dc_base removed on this PR, too, if we're removing references to it.

But why remove references to it? Is it so bad to have that abstracted?

Copy link
Member Author

Choose a reason for hiding this comment

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

$dc_base is removed, but I can keep it too if it's useful to keep that abstracted. The reason I removed it is because it seemed to only be used to determine if docker compose or docker-compose should be used

Copy link
Member

Choose a reason for hiding this comment

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

+1 for removal for now.

Copy link
Member

Choose a reason for hiding this comment

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

I bet we're going to hear about this from users. 🐭

if [[ -z "$COMPOSE_VERSION" ]]; then
echo "FAIL: Docker compose is required to run self-hosted"
exit 1
fi

if [[ "$(vergte ${COMPOSE_VERSION//v/} $MIN_COMPOSE_VERSION)" -eq 1 ]]; then
echo "FAIL: Expected minimum $dc_base version to be $MIN_COMPOSE_VERSION but found $COMPOSE_VERSION"
echo "FAIL: Expected minimum docker compose version to be $MIN_COMPOSE_VERSION but found $COMPOSE_VERSION"
exit 1
fi
echo "Found Docker Compose version $COMPOSE_VERSION"
Expand Down
6 changes: 2 additions & 4 deletions install/dc-detect-version.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@ fi

echo "${_group}Initializing Docker Compose ..."

# Some environments still use `docker-compose` even for Docker Compose v2.
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this ever true? I think some environments allow docker-compose to be used even on Docker Compose v2, but the official syntax should be docker compose instead of docker-compose. At least specified in the docs:
https://docs.docker.com/compose/migrate/

Copy link
Member

Choose a reason for hiding this comment

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

We found it to be true in the wild, hence the accommodation. Could dig in history and find the PR for more detail. It was a linux distro that munged the name, don't remember which.

Copy link

Choose a reason for hiding this comment

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

See https://docs.docker.com/compose/install/standalone/

For example type docker-compose up when using Compose standalone, instead of docker compose up.

dc_base="$(docker compose version &>/dev/null && echo 'docker compose' || echo 'docker-compose')"
Copy link
Collaborator

@aminvakil aminvakil Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A change in docker compose > 2 which caused some users complaining about it, was that it couldn't run as a standalone binary anymore. And maintainer's response was to make it executable.

So the only docker compose which could not get run as a executable was 2.0.0 and any docker compose >= 2.0.1 can get run as executable.

And some users (not sure how many) still rely on docker-compose and make /usr/bin/docker-compose or /usr/local/bin/docker-compose a symlink to /usr/lib/docker/cli-plugins/docker-compose, so they can just run docker-compose and not docker compose.

So some distributions (at least one I know :) ) still create this symlink.

Discussion about this should be done or not, what (docker compose or docker-compose) should get used or best practices about this is out of scope of this repository IMHO, but keeping this so users which are still relying on docker-compose existence and use it instead of docker compose is not something hard to maintain in future.

So I advise keeping this line and reverting s/$dc_base/docker compose/ below.

P.S. I have switched to docker compose myself:)

Copy link
Member Author

Choose a reason for hiding this comment

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

fair enough, we can keep this around then 👍

if [[ "$(basename $0)" = "install.sh" ]]; then
dc="$dc_base --ansi never --env-file ${_ENV}"
dc="docker compose --ansi never --env-file ${_ENV}"
else
dc="$dc_base --ansi never"
dc="docker compose --ansi never"
fi
proxy_args="--build-arg http_proxy=${http_proxy:-} --build-arg https_proxy=${https_proxy:-} --build-arg no_proxy=${no_proxy:-}"
dcr="$dc run --rm"
Expand Down
2 changes: 1 addition & 1 deletion install/set-up-and-migrate-database.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ if [[ -n "${CI:-}" || "${SKIP_USER_CREATION:-0}" == 1 ]]; then
echo "Did not prompt for user creation. Run the following command to create one"
echo "yourself (recommended):"
echo ""
echo " $dc_base run --rm web createuser"
echo " docker compose run --rm web createuser"
echo ""
else
$dcr web upgrade
Expand Down
4 changes: 2 additions & 2 deletions install/wrap-up.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ else
echo "You're all done! Run the following command to get Sentry running:"
echo ""
if [[ "${_ENV}" =~ ".env.custom" ]]; then
echo " $dc_base --env-file ${_ENV} up -d"
echo " docker compose --env-file ${_ENV} up -d"
else
echo " $dc_base up -d"
echo " docker compose up -d"
fi
echo ""
echo "-----------------------------------------------------------------"
Expand Down