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 publish-all cli flag #2545

Merged
merged 1 commit into from
Dec 17, 2018

Conversation

ZYecho
Copy link
Contributor

@ZYecho ZYecho commented Dec 8, 2018

Signed-off-by: zhangyue zy675793960@yeah.net

Ⅰ. Describe what this PR did

add cli publish all flag to support publish all expose port.

Ⅱ. Does this pull request fix one issue?

fix #2517

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

add a integration test.

Ⅳ. Describe how to verify it

pouch run -d -P redis
to get a publish all container.
pouch inspect cefab5
Got the random port

 "NetworkSettings": {
            "Networks": {
                "bridge": {
                    "Aliases": null,
                    "EndpointID": "3cf3f181d5066cc607a64faaf1149fe2745fc607781cd071c499aa8fbf696931",
                    "Gateway": "192.168.5.1",
                    "IPAddress": "192.168.5.2",
                    "IPPrefixLen": 24,
                    "Links": null,
                    "MacAddress": "02:42:c0:a8:05:02",
                    "NetworkID": "12541cab332aaabf2695006c547441d38b46fb94f857db65a491165acedd1518"
                }
            },
            "Ports": {
                "6379/tcp": [
                    {
                        "HostIp": "0.0.0.0",
                        "HostPort": "32768"
                    }
                ]
            },
            "SandboxID": "5f2e05d9b731ac979191c6c8340dbccaa68a784ccaf1e735cc6ea4e07aff2a54",
            "SandboxKey": "/var/run/pouch/netns/5f2e05d9b731",
            "SecondaryIPAddresses": null,
            "SecondaryIPv6Addresses": null
        },

use lsof to verify
lsof -iTCP | grep pouch
Got
pouchd 15090 root 16u IPv6 265330 0t0 TCP *:32768 (LISTEN)

Ⅴ. Special notes for reviews

None.

@codecov
Copy link

codecov bot commented Dec 8, 2018

Codecov Report

Merging #2545 into master will decrease coverage by 0.16%.
The diff coverage is 72.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2545      +/-   ##
==========================================
- Coverage   69.18%   69.02%   -0.17%     
==========================================
  Files         278      278              
  Lines       18565    18607      +42     
==========================================
- Hits        12844    12843       -1     
- Misses       4262     4293      +31     
- Partials     1459     1471      +12
Flag Coverage Δ
#criv1alpha1test 31.29% <39.53%> (-0.01%) ⬇️
#criv1alpha2test 35.6% <39.53%> (+0.06%) ⬆️
#integrationtest 40.57% <72.09%> (-0.06%) ⬇️
#nodee2etest 32.58% <30.23%> (-0.06%) ⬇️
#unittest 26.77% <0%> (-0.07%) ⬇️
Impacted Files Coverage Δ
daemon/mgr/container.go 58.7% <100%> (+0.05%) ⬆️
daemon/mgr/network.go 72.9% <66.66%> (-0.06%) ⬇️
cri/ocicni/cni_manager.go 58.82% <0%> (-11.77%) ⬇️
daemon/mgr/system.go 67.93% <0%> (-5.35%) ⬇️
ctrd/watch.go 80.28% <0%> (-4.23%) ⬇️
apis/server/utils.go 71.15% <0%> (-3.85%) ⬇️
daemon/mgr/events.go 96.29% <0%> (-3.71%) ⬇️
ctrd/client.go 66.92% <0%> (-2.31%) ⬇️
ctrd/image.go 76.75% <0%> (-2.2%) ⬇️
cri/v1alpha2/cri_wrapper.go 65.59% <0%> (-1.21%) ⬇️
... and 7 more

@ZYecho ZYecho force-pushed the add-publishall branch 6 times, most recently from c673c9c to 573e296 Compare December 10, 2018 01:48
@allencloud allencloud changed the title feature: addd publish-all cli flag feature: add publish-all cli flag Dec 10, 2018
daemon/mgr/network.go Outdated Show resolved Hide resolved
@ZYecho ZYecho force-pushed the add-publishall branch 2 times, most recently from 4b709ef to ff44374 Compare December 11, 2018 01:28
daemon/mgr/network.go Outdated Show resolved Hide resolved
@allencloud
Copy link
Collaborator

This is the only CI failure in integration test:

----------------------------------------------------------------------
FAIL: /home/travis/gopath/src/github.com/alibaba/pouch/test/cli_inspect_test.go:195: PouchInspectSuite.TestContainerInspectPorts
/home/travis/gopath/src/github.com/alibaba/pouch/test/cli_inspect_test.go:208:
    c.Assert(string(data), check.Equals, "{\"80/tcp\":[{\"HostPort\":\"8080\"}]}")
... obtained string = "{\"80/tcp\":[{\"HostIp\":\"0.0.0.0\",\"HostPort\":\"8080\"}]}"
... expected string = "{\"80/tcp\":[{\"HostPort\":\"8080\"}]}"
----------------------------------------------------------------------

@ZYecho I think it is related to the code change.

@ZYecho ZYecho force-pushed the add-publishall branch 2 times, most recently from 88272eb to ca1bfbc Compare December 12, 2018 02:06
@ZYecho
Copy link
Contributor Author

ZYecho commented Dec 12, 2018

This is the only CI failure in integration test:

----------------------------------------------------------------------
FAIL: /home/travis/gopath/src/github.com/alibaba/pouch/test/cli_inspect_test.go:195: PouchInspectSuite.TestContainerInspectPorts
/home/travis/gopath/src/github.com/alibaba/pouch/test/cli_inspect_test.go:208:
    c.Assert(string(data), check.Equals, "{\"80/tcp\":[{\"HostPort\":\"8080\"}]}")
... obtained string = "{\"80/tcp\":[{\"HostIp\":\"0.0.0.0\",\"HostPort\":\"8080\"}]}"
... expected string = "{\"80/tcp\":[{\"HostPort\":\"8080\"}]}"
----------------------------------------------------------------------

@ZYecho I think it is related to the code change.

Thanks, fixed it.

Copy link
Collaborator

@rudyfly rudyfly left a comment

Choose a reason for hiding this comment

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

Just a litter suggestions.

daemon/mgr/container.go Outdated Show resolved Hide resolved
daemon/mgr/container.go Outdated Show resolved Hide resolved
daemon/mgr/container.go Show resolved Hide resolved
test/cli_run_network_test.go Show resolved Hide resolved
Signed-off-by: zhangyue <zy675793960@yeah.net>
@rudyfly
Copy link
Collaborator

rudyfly commented Dec 17, 2018

LGTM
Wait ci

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 17, 2018
@allencloud
Copy link
Collaborator

Thanks a lot for your work. @ZYecho
Merging...

@allencloud allencloud merged commit 73256bf into AliyunContainerService:master Dec 17, 2018
@ZYecho ZYecho deleted the add-publishall branch April 27, 2019 02:06
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/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature-request] support publishAll flag in container's command line
4 participants