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

-p bug #1962 #2041

Closed
wants to merge 4 commits into from
Closed

-p bug #1962 #2041

wants to merge 4 commits into from

Conversation

achhapolia10
Copy link

@achhapolia10 achhapolia10 commented Aug 11, 2019

Closes #1962
Signed-off-by: Anshuman Chhapolia achhap.10.01@gmail.com

- What I did
After Fixing the bug docker command gives an appropriate result for invalid ports.

$ ./docker run -p 808080:80 node
./docker: Invalid hostPort: 808080.
See './docker run --help'.

It also allows for mixed notations for port publishing in long and short formats.

$ ./docker run -d -p 8080:80 -p target=4040,published=443 node

- How I did it
Instead of expecting the notation in the shorthand format and if it gives an error and then parsing the long format, parse all the items and checked for '=' if an item contains '=' we can surely say that its a long format. Then I converted the long formats to shorthand format.

- How to verify it
Function TestParsePublishPorts() added.
Build and tested the binaries myself. Examples are shown above.

- Description for the changelog
Fixed #1962 as suggested by @thaJeztah

image

Signed-off-by: Anshuman Chhapolia <achhap.10.01@gmail.com>
Signed-off-by: Anshuman Chhapolia <achhap.10.01@gmail.com>
Signed-off-by: Anshuman Chhapolia <achhap.10.01@gmail.com>
Signed-off-by: Anshuman Chhapolia <achhap.10.01@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #2041 into master will increase coverage by 0.05%.
The diff coverage is 64.28%.

@@            Coverage Diff             @@
##           master    #2041      +/-   ##
==========================================
+ Coverage   56.79%   56.85%   +0.05%     
==========================================
  Files         311      311              
  Lines       21836    21837       +1     
==========================================
+ Hits        12402    12415      +13     
+ Misses       8519     8505      -14     
- Partials      915      917       +2

@achhapolia10 achhapolia10 changed the title 1962 parse publish -p bug #1962 Aug 12, 2019
@achhapolia10
Copy link
Author

#2041 closes #839 too.
Command to test it

$ ./docker run --name node -p target=4040-4042,published=4040-4042 -dit --rm node

docker ps results

CONTAINER ID        IMAGE               COMMAND                  CREATED             STATUS              PORTS                              NAMES
c7ab56f8366a        node                "docker-entrypoint.s…"   8 seconds ago       Up 6 seconds        0.0.0.0:4040-4042->4040-4042/tcp   node

@thaJeztah
Copy link
Member

Thank you for contributing; there were a couple of pull-requests addressing the same issue, and after reviewing those, we picked #1965 (carried in #2251)

Apologies that your PR didn't get merged, but we appreciate your contribution (and hope to see future contributions 🤗 )

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.

Port out of bounds gives error 'invalid publish opts format'
4 participants