pelias / docker

Run the Pelias geocoder in docker containers, including example projects.
MIT License
314 stars 217 forks source link

support modern docker compose #317

Closed michaelkirk closed 7 months ago

michaelkirk commented 1 year ago

Here's the reason for this change :rocket:

Fixes error like:

line 20: docker-compose: command not found

From: https://docs.docker.com/compose/gettingstarted/

Compose V1 no longer receives updates and will not be available in new releases of Docker Desktop after June 2023.


Here's what actually got changed :clap:

Alternatively we could just drop docker-compose (v1) support. I confess that I didn't fully test the fallback behavior anyway, and apparently it's no longer supported by docker.


Here's how others can test the changes :eyes:

Run through any of the example projects that utilize a variety of docker compose commands, e.g. portland-metro

closes https://github.com/pelias/docker/pull/303

missinglink commented 1 year ago

Possible duplicate of https://github.com/pelias/docker/pull/316

michaelkirk commented 1 year ago

Possible duplicate of #303 as well, but this PR also includes some relevant doc updates.

I'd be happy to see either of those merged as an alternative, and I could follow up with the doc changes separately.

missinglink commented 7 months ago

Hi @michaelkirk is this working for you? I went to test it today and found it produced an error:

docker compose version
Docker Compose version v2.19.1
STRING='docker compose'

$STRING version
zsh: command not found: docker compose

The other PR seems to suffer the same issue:

DOCKER_COMPOSE_COMMAND="docker compose"

function compose_exec(){ ${DOCKER_COMPOSE_COMMAND} $@; }

compose_exec version
compose_exec: command not found: docker compose

The shell is expecting a single word for the command followed by zero or more arguments, I don't believe it's possible to have a multi-word command such as this.

missinglink commented 7 months ago

How about this?

function compose_exec(){
    if ! command -v docker-compose &> /dev/null
    then docker compose $@;
    else docker-compose $@;
    fi
}
missinglink commented 7 months ago

actually, this is probably nicer:

# the 'docker compose' subcommand is now the recommended method of calling compose.
# if not available, we fallback to the legacy 'docker-compose' command.
function compose_exec(){
  NATIVE_COMPOSE_VERSION=$(docker compose version 2> /dev/null || true)
  if [ -z "$NATIVE_COMPOSE_VERSION" ]; then
    docker-compose $@;
  else
    docker compose $@;
  fi
}

It has the advantage of not causing the process to exit if the shell is running with set -x plus it favours the newer compose method.

It works by attempting to execute docker compose version and then checking if the output was an empty string or not.

I considered emitting a warning where docker-compose was installed on the system, but modern versions of Docker Desktop seem to create a symlink which makes that method fragile.

missinglink commented 7 months ago

Hi @michaelkirk I've pushed a commit to your branch, could you please test it and let me know if it's good to merge?

michaelkirk commented 7 months ago

Yes, I've been using this code. The error you note seems to be a difference between bash and zsh. Note that cmd/docker.sh (and the ./pelias executable) has a shebang: #!/bin/bash

I can indeed reproduce the error you encountered if I change the script to instead run with zsh.

In any case, I'm fine with the version you have pushed as well. Thanks for reviewing!