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

feature: add port mapping for container #833

Merged
merged 1 commit into from
Mar 8, 2018

Conversation

rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented Mar 7, 2018

Ⅰ. Describe what this PR did

Add port mapping for container. You can use:
-p or --port to port binging into container
--expose to expose container's port to access.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

# pouch run -d -p 8888:80 registry.hub.docker.com/library/httpd

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang rudyflyzhang@gmail.com

Add port mapping for container. You can use:
-p or --port to port binging into container
--expose to expose container's port to access.

    pouch run -d -p 8888:80 registry.docker.com/library/nginx:latest

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
c.Assert(err, check.IsNil)

command.PouchRun("rm", "-f", funcname)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

good test!

@Letty5411
Copy link
Contributor

CI failed due to know issue #815

@Letty5411
Copy link
Contributor

@rudyfly could you also add test to check the exposed port works?

@codecov-io
Copy link

Codecov Report

Merging #833 into 0.2.x will decrease coverage by 0.15%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##            0.2.x     #833      +/-   ##
==========================================
- Coverage   13.77%   13.62%   -0.16%     
==========================================
  Files         107      107              
  Lines        6460     6534      +74     
==========================================
  Hits          890      890              
- Misses       5511     5585      +74     
  Partials       59       59
Impacted Files Coverage Δ
cli/common_flags.go 0% <0%> (ø) ⬆️
daemon/mgr/network.go 3.95% <0%> (-0.62%) ⬇️
daemon/mgr/container.go 2.85% <0%> (-0.01%) ⬇️
cli/container.go 41.2% <0%> (-5.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f2a278...92c5992. Read the comment docs.

@@ -131,18 +135,61 @@ func (c *container) config() (*types.ContainerCreateConfig, error) {
}
}

// parse port binding
Copy link
Collaborator

@allencloud allencloud Mar 8, 2018

Choose a reason for hiding this comment

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

In the future, we could encapsulate the following code block into a function like parsePortMapping

func parsePortMapping(ports []string)(ports [string]nats, portBindings xxxx)

maybe has the incorrect type, but the similar meaning. In addition, this is much easier to write unit test cases.

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Mar 8, 2018
@allencloud allencloud merged commit 415badc into AliyunContainerService:0.2.x Mar 8, 2018
@rudyfly rudyfly deleted the 0.2.x_portmapping branch October 29, 2018 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants