cli/cli: use len() to check frontend ports in the port command#6063
cli/cli: use len() to check frontend ports in the port command#6063thaJeztah merged 1 commit intodocker:masterfrom
len() to check frontend ports in the port command#6063Conversation
This ensure the command won't print an empty output if the `frontends` port is nil Signed-off-by: Giau. Tran Minh <hello@giautm.dev>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6063 +/- ##
=======================================
Coverage 55.15% 55.15%
=======================================
Files 358 358
Lines 30023 30023
=======================================
Hits 16558 16558
Misses 12519 12519
Partials 946 946 🚀 New features to boost your workflow:
|
|
@thaJeztah Sorry to bother you, please help me review this. |
len() to check frontends ports in the port commandlen() to check frontend ports in the port command
|
Do you have more information about the problem you ran into? Looking at the code, it's expected to not return an error unless querying for a specific port; cli/cli/command/container/port.go Lines 62 to 73 in 518ba2b The code-change overall looks harmless, but I'm curious in what situation you ran into where it didn't work as expected. With the current version of the CLI; docker run -d --name mycontainer nginx:alpine
docker port mycontainer
# no output and no error
docker port mycontainer 80
Error: No public port '80' published for mycontainer |
I've updated the reason in PR's description:
I was unable to reproduce this issue yet, my willing guess it's related to OS / Docker Engine that unable to allocate a free port right after the container started. fyi, I started the container with And right after the container created (it prints the ID), the command |
|
Hm, gotcha! Was that on Docker Desktop or was it a docker installed on Linux? (On Docker Desktop, there's some additional steps involved in mapping ports, so wondering if that could play a role, resulting in a race condition). |
|
Either way; the change in this PR looks fine to me, so let me approve and merge |
That's Docker Desktop and I invoke the command inside WSL. |
|
Thanks! cc @akerouanton @robmry FYI (port-mapping) |
- What I did
I ran into the case that
docker portreturn an empty string and exist successfully, checking the code base, I found this function check the frontend ports usingnilcheck - which may incorrect in the case of empty slice.So, this PR change to logic to use
len(frontends) == 0for check ports. If the slice is empty (nil or non-nil) it both return an error.- How I did it
use
len()to check instead ofnilcheck- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)
