fix: Podman compose incorrectly targets docker engine w/ compose - #386
fix: Podman compose incorrectly targets docker engine w/ compose#386muchzill4 wants to merge 5 commits into
Conversation
Signed-off-by: Bartek Mucha <bartosz.mucha@arm.com>
Signed-off-by: Bartek Mucha <bartosz.mucha@arm.com>
Signed-off-by: Bartek Mucha <bartosz.mucha@arm.com>
Signed-off-by: Bartek Mucha <bartosz.mucha@arm.com>
Signed-off-by: Bartek Mucha <bartosz.mucha@arm.com>
th3james
left a comment
There was a problem hiding this comment.
Functionally looks OK, but there are a few tweaks to consider
| runtime_dir="$RUNNER_TEMP/podman-runtime" | ||
| mkdir -p "$runtime_dir/podman" | ||
| chmod 700 "$runtime_dir" | ||
| echo "XDG_RUNTIME_DIR=$runtime_dir" >> "$GITHUB_ENV" | ||
| export XDG_RUNTIME_DIR="$runtime_dir" |
There was a problem hiding this comment.
Overwriting this seems a touch invasive and could let to subtle misbehaviour. Possible that I'm being paranoid, but is it worth respecting the existing value?
| runtime_dir="$RUNNER_TEMP/podman-runtime" | |
| mkdir -p "$runtime_dir/podman" | |
| chmod 700 "$runtime_dir" | |
| echo "XDG_RUNTIME_DIR=$runtime_dir" >> "$GITHUB_ENV" | |
| export XDG_RUNTIME_DIR="$runtime_dir" | |
| runtime_dir="${XDG_RUNTIME_DIR:-$RUNNER_TEMP/podman-runtime}" | |
| mkdir -p "$runtime_dir/podman" | |
| if [ -z "${XDG_RUNTIME_DIR:-}" ]; then | |
| chmod 700 "$runtime_dir" | |
| echo "XDG_RUNTIME_DIR=$runtime_dir" >> "$GITHUB_ENV" | |
| export XDG_RUNTIME_DIR="$runtime_dir" | |
| fi |
| func (socket Socket) ConfigureComposeEnv(environment []string) ([]string, error) { | ||
| if socket.localComposeSocket == nil { | ||
| return append(environment, "DOCKER_HOST="+socket.url), nil | ||
| } | ||
| return socket.localComposeSocket.ConfigureEnv(environment) | ||
| } | ||
|
|
||
| func (socket *localComposeSocket) ConfigureEnv(environment []string) ([]string, error) { | ||
| socket.mutex.Lock() | ||
| defer socket.mutex.Unlock() | ||
|
|
||
| if socket.url == "" { | ||
| url, err := ResolveLocalComposeSocket(context.Background()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| socket.url = url | ||
| } | ||
| return append(environment, "DOCKER_HOST="+socket.url), nil | ||
| } |
There was a problem hiding this comment.
Found this quite hard to follow, I think it's clearer if we separate out "get the URL from a Socket" from "change the ENV"
| func (socket Socket) ConfigureComposeEnv(environment []string) ([]string, error) { | |
| if socket.localComposeSocket == nil { | |
| return append(environment, "DOCKER_HOST="+socket.url), nil | |
| } | |
| return socket.localComposeSocket.ConfigureEnv(environment) | |
| } | |
| func (socket *localComposeSocket) ConfigureEnv(environment []string) ([]string, error) { | |
| socket.mutex.Lock() | |
| defer socket.mutex.Unlock() | |
| if socket.url == "" { | |
| url, err := ResolveLocalComposeSocket(context.Background()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| socket.url = url | |
| } | |
| return append(environment, "DOCKER_HOST="+socket.url), nil | |
| } | |
| func (socket Socket) ConfigureComposeEnv(environment []string) ([]string, error) { | |
| socketURL, err := socket.GetSocketURL() | |
| if err != nil { | |
| return nil, err | |
| } | |
| return append(environment, "DOCKER_HOST="+socketURL), nil | |
| } | |
| func (socket Socket) GetSocketURL() (string, error) { | |
| if socket.localComposeSocket != nil { | |
| return socket.localComposeSocket.GetSocketURL() | |
| } | |
| if socket.url == "" { | |
| return "", errors.New("podman socket URL not configured") | |
| } | |
| return socket.url, nil | |
| } | |
| func (socket *localComposeSocket) GetSocketURL() (string, error) { | |
| socket.mutex.Lock() | |
| defer socket.mutex.Unlock() | |
| if socket.url == "" { | |
| url, err := ResolveLocalComposeSocket(context.Background()) | |
| if err != nil { | |
| return "", err | |
| } | |
| socket.url = url | |
| } | |
| return socket.url, nil | |
| } |
|
|
||
| // ResolveLocalComposeSocket returns the host-accessible Podman API endpoint | ||
| // that the external Compose provider must use for a local deployment. | ||
| func ResolveLocalComposeSocket(ctx context.Context) (string, error) { |
There was a problem hiding this comment.
This is on the public interface and it takes a context, but it has a single call site inside this package , and the only context it gets passed is context.Background().
Should we consider either removing the ability to pass a context, or update the function APIs up the call stack to allow passing the existing context down?
Changes
ℹ️ This seems to affect podman 4.9.x, which is the version ubuntu 24.04 LTS ship with, my local tests with brewed podman 6.0.0 did work as expected w/o explicit
DOCKER_HOSTbeing setdocker-composeprovider with Podman’s resolved local API endpoint viaDOCKER_HOST.CONTAINER_HOSTonly for explicit sockets.Checklist