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

Improve presentation of published port ranges #581

Merged
merged 1 commit into from
Jan 16, 2018

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Oct 2, 2017

fixes #178

Port mappings in docker service ls are quite verbose, and occupy a lot of
space when ranges of ports are published.

This patch improves the output by reconstructing ranges of ports.

Given the following service;

$ docker service create \
  -p 60-61:60-61 \
  -p 62:61 \
  -p 80:80 \
  -p 81:80 \
  -p 90-95:90-95 \
  -p 90-92:90-92/udp \
  -p 93-96:93-96/udp \
  --name foo \
  nginx:alpine

Before this patch is applied:

$ docker service ls
ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
u1kwguv841qg        foo                 replicated          1/1                 nginx:alpine        *:60->60/tcp,*:61->61/tcp,*:62->61/tcp,*:80->80/tcp,*:81->80/tcp,*:90->90/tcp,*:91->91/tcp,*:92->92/tcp,*:93->93/tcp,*:94->94/tcp,*:95->95/tcp,*:90->90/udp,*:91->91/udp,*:92->92/udp,*:93->93/udp,*:94->94/udp,*:95->95/udp,*:96->96/udp

After this patch is applied:

$ docker service ls
ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
u1kwguv841qg        foo                 replicated          1/1                 nginx:alpine        *:60-62->60-61/tcp,*:80-81->80/tcp,*:90-95->90-95/tcp,*:90-96->90-96/udp

Additional enhancements can still be made, and marked as TODO in this change;

  • combine non-consecutive ports mapped to a single port (80->80, 81->80,
    84->80, 86->80, 87->80); to be printed as *:80-81,84,86-87->80.
  • combine tcp and udp mappings if their port-mapping is the same;
    print *:80-81->80-81/tcp+udp instead of *:80-81->80-81/tcp, *:80-81->80-81/udp

- Description for the changelog

* Improve presentation of published ports by combining port-ranges [docker/cli#581](https://github.com/docker/cli/pull/581)

Port mappings in `docker service ls` are quite verbose, and occupy a lot of
space when ranges of ports are published.

This patch improves the output by reconstructing ranges of ports.

Given the following service;

    $ docker service create \
      -p 60-61:60-61 \
      -p 62:61 \
      -p 80:80 \
      -p 81:80 \
      -p 90-95:90-95 \
      -p 90-92:90-92/udp \
      -p 93-96:93-96/udp \
      --name foo \
      nginx:alpine

Before this patch is applied:

    $ docker service ls
    ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
    u1kwguv841qg        foo                 replicated          1/1                 nginx:alpine        *:60->60/tcp,*:61->61/tcp,*:62->61/tcp,*:80->80/tcp,*:81->80/tcp,*:90->90/tcp,*:91->91/tcp,*:92->92/tcp,*:93->93/tcp,*:94->94/tcp,*:95->95/tcp,*:90->90/udp,*:91->91/udp,*:92->92/udp,*:93->93/udp,*:94->94/udp,*:95->95/udp,*:96->96/udp

After this patch is applied:

    $ docker service ls
    ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
    u1kwguv841qg        foo                 replicated          1/1                 nginx:alpine        *:60-62->60-61/tcp,*:80-81->80/tcp,*:90-95->90-95/tcp,*:90-96->90-96/udp

Additional enhancements can still be made, and marked as TODO in this change;

- combine non-consecutive ports mapped to a single port (`80->80`, `81->80`,
  `84->80`, `86->80`, `87->80`); to be printed as `*:80-81,84,86-87->80`.
- combine `tcp` and `udp` mappings if their port-mapping is the same;
  print `*:80-81->80-81/tcp+udp` instead of `*:80-81->80-81/tcp, *:80-81->80-81/udp`

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

ping @dnephin @vdemeester @jefflill PTAL

While working on this, I noticed that various commands include the formatted values when using --format '{{ json . }}'. I think we should look at that, and always print the raw values when printing JSON (not sure there's a way to do that on a "global" level, or if that has to be fixed for each command separately.

@codecov-io
Copy link

codecov-io commented Oct 2, 2017

Codecov Report

Merging #581 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #581      +/-   ##
==========================================
+ Coverage   49.37%   49.52%   +0.14%     
==========================================
  Files         208      208              
  Lines       17178    17225      +47     
==========================================
+ Hits         8482     8530      +48     
+ Misses       8263     8262       -1     
  Partials      433      433

@thaJeztah
Copy link
Member Author

ping @dnephin @vdemeester 🤗

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

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.

[docker service ls] should recognize and format published port ranges
5 participants