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

support modern docker compose #317

Merged

Conversation

michaelkirk
Copy link
Contributor

@michaelkirk michaelkirk commented Jun 1, 2023

Here's the reason for this change 🚀

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 👏

  • update docker compose docs
  • refreshed links related to docker compose
    • all the docker commands now delegate to compose_exec (previously only half did)
  • compose_exec now first tries docker compose before falling back to docker-compose`

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 👀

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

closes #303

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.

Fixes error like:

	line 20: docker-compose: command not found
@missinglink
Copy link
Member

Possible duplicate of #316

@michaelkirk
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

How about this?

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

@missinglink
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

michaelkirk commented Nov 29, 2023

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!

@missinglink missinglink merged commit 5848b66 into pelias:master Nov 29, 2023
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.

2 participants