Skip to content

Conversation

@ZYecho
Copy link
Contributor

@ZYecho ZYecho commented Dec 8, 2018

Signed-off-by: zhangyue [email protected]

Ⅰ. 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
@ZYecho ZYecho force-pushed the add-publishall branch 2 times, most recently from 4b709ef to ff44374 Compare December 11, 2018 01:28
@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.

@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