Skip to content

fixes "docker port <id> portrange" #1396

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

lifubang
Copy link
Contributor

@lifubang lifubang commented Sep 28, 2018

Signed-off-by: Lifubang lifubang@acmcoder.com

- What I did
I have a container redis2:

docker run -d --name=redis2 -p 6370-6373 -p 6376-6379 redis

When run docker port redis2 6376-6379, it alway said:

Error: No public port '6376-6379/tcp' published for redis2

Because in most of time, there are no port range key in ports of docker inspect result .
So, I think we can get the port info separately.

- How I did it
Don't return an error when '6376-6379/tcp' not found.
then use portStart, portEnd, err := nat.ParsePortRangeToInt(port)
then print port info from portStart to portEnd

- How to verify it

root@dockerdemo:~/gocode/src/github.com/docker/cli# ./build/docker port redis2 6376-6379
6376/tcp -> 0.0.0.0:32781
6377/tcp -> 0.0.0.0:32780
6378/tcp -> 0.0.0.0:32779
6379/tcp -> 0.0.0.0:32778
root@dockerdemo:~/gocode/src/github.com/docker/cli# ./build/docker port redis2 6371-6380
6371/tcp -> 0.0.0.0:32784
6372/tcp -> 0.0.0.0:32783
6373/tcp -> 0.0.0.0:32782
Error: No public port '6374/tcp' published for redis2
Error: No public port '6375/tcp' published for redis2
6376/tcp -> 0.0.0.0:32781
6377/tcp -> 0.0.0.0:32780
6378/tcp -> 0.0.0.0:32779
6379/tcp -> 0.0.0.0:32778
Error: No public port '6380/tcp' published for redis2

- Description for the changelog
Make port range in docker port can work.

- A picture of a cute animal
2zga8u izj6 5qq7k38kuv

@lifubang
Copy link
Contributor Author

I also think that the message "Error: No public port '6376-6379/tcp' published for redis2" is a bug, because it have public port for them.

@lifubang lifubang changed the title enhancement of docker port <id> ports-porte fixes "docker port <id> portrange" Sep 28, 2018
@codecov-io
Copy link

codecov-io commented Sep 28, 2018

Codecov Report

Merging #1396 into master will increase coverage by 0.06%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1396      +/-   ##
==========================================
+ Coverage   54.31%   54.37%   +0.06%     
==========================================
  Files         289      289              
  Lines       19318    19274      -44     
==========================================
- Hits        10492    10481      -11     
+ Misses       8148     8121      -27     
+ Partials      678      672       -6

Signed-off-by: Lifubang <lifubang@acmcoder.com>
@lifubang
Copy link
Contributor Author

@kolyshkin If you have a time, please take a look this one.

@lifubang
Copy link
Contributor Author

Or disable port range in docker port?

fmt.Fprintf(dockerCli.Out(), "%s -> %s:%s\n", nowP, frontend.HostIP, frontend.HostPort)
}
} else {
fmt.Fprintf(dockerCli.Out(), "Error: No public port '%s' published for %s\n", nowP, opts.container)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe

return errors.Errorf("no public port %q published for %s", nowP, opts.container)

?

portStart, portEnd, _ := nat.ParsePortRange(port)
if portEnd > portStart {
for nport := portStart; nport <= portEnd; nport++ {
nowP, _ := nat.NewPort(proto, fmt.Sprint(nport))
Copy link
Contributor

Choose a reason for hiding this comment

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

strconv.Itoa(nport)?

@kolyshkin
Copy link
Contributor

Or disable port range in docker port?

docker port synopsis is docker port CONTAINER [PRIVATE_PORT[/PROTO]]. You can either disallow port range (i.e. try to convert port to integer and error with "invalid port: %s", or allow port ranges like you did in this patch, but if you're going that route, you need to modify documentation as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants