Skip to content

Support SCTP port mapping - #278

Merged
vdemeester merged 1 commit into
docker:masterfrom
ishidawataru:sctp
Feb 21, 2018
Merged

Support SCTP port mapping#278
vdemeester merged 1 commit into
docker:masterfrom
ishidawataru:sctp

Conversation

@ishidawataru

@ishidawataruishidawataru commented Jul 3, 2017

Copy link
Copy Markdown
Contributor

Signed-off-by: Wataru Ishida ishida.wataru@lab.ntt.co.jp

- What I did

This PR add support for SCTP port mapping.

Please see moby/moby#33922 for the details.

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@codecov-io

codecov-io commented Jul 3, 2017•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #278 into master will not change coverage.
The diff coverage is 100%.

@@ Coverage Diff @@## master #278 +/- ##
=======================================
Coverage 53.21% 53.21% =======================================
Files 258 258 Lines 16346 16346 =======================================
Hits 8699 8699 Misses 7082 7082 Partials 565 565

@ishidawataruishidawataru mentioned this pull request Jul 3, 2017
Merged
4 tasks
@aaronlehmann

aaronlehmann commented Jul 7, 2017

Copy link
Copy Markdown

I see the PR in go-connections is docker/go-connections#41

Let's merge that first, then we can update this PR to vendor the actual go-connections repository instead of a fork.

@ishidawataru

ishidawataru commented Jul 11, 2017

Copy link
Copy Markdown
ContributorAuthor

@aaronlehmann Sure. I'll update this PR after docker/go-connections#41 get merged.

However, I think moby/libnetwork#1825 is the first PR we need to merge for SCTP port mapping feature.
Could you also take a look on that PR?

@AkihiroSuda

AkihiroSuda commented Feb 2, 2018

Copy link
Copy Markdown
Collaborator

rebased

@dnephin

dnephin commented Feb 2, 2018

Copy link
Copy Markdown
Contributor

Ok, but let's not merge this until the moby/moby PR is merged.

@AkihiroSuda
AkihiroSudaforce-pushed the sctp branch 2 times, most recently from a224f0e to 513e958CompareFebruary 16, 2018 04:20
@thaJeztah

thaJeztah commented Feb 16, 2018

Copy link
Copy Markdown
Member

Perhaps we want to update the tests for #581 to include SCTP

@thaJeztah

thaJeztah commented Feb 20, 2018

Copy link
Copy Markdown
Member

moby/moby#33922 was merged

dnephin
dnephin approved these changes Feb 20, 2018
View reviewed changes

@dnephindnephin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

cpuguy83
cpuguy83 reviewed Feb 20, 2018
View reviewed changes
Comment threadopts/port.go Outdated
switch key {
case portOptProtocol:
if value != string(swarm.PortConfigProtocolTCP) && value != string(swarm.PortConfigProtocolUDP) {
// TODO: use swarm.PortConfigProtocolSCTP (vendor moby/moby#33922)

@cpuguy83cpuguy83Feb 20, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks like we just need to vendor this in now?

@thaJeztah

thaJeztah commented Feb 20, 2018

Copy link
Copy Markdown
Member

Opened a vendor PR for moby/moby: #892

@AkihiroSuda

AkihiroSuda commented Feb 20, 2018

Copy link
Copy Markdown
Collaborator

Will rebase soon after #892 gets merged, thanks

Wataru Ishida@AkihiroSuda
Support SCTP port mapping
995006c
Signed-off-by: Wataru Ishida <ishida.wataru@lab.ntt.co.jp>
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda

AkihiroSuda commented Feb 21, 2018

Copy link
Copy Markdown
Collaborator

done

vdemeester
vdemeester approved these changes Feb 21, 2018
View reviewed changes

@vdemeestervdemeester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 🐯

@vdemeester
vdemeester merged commit 64f92fd into docker:masterFeb 21, 2018
@GordonTheTurtleGordonTheTurtle added this to the 18.03.0 milestone Feb 21, 2018
@thaJeztahthaJeztah mentioned this pull request Feb 21, 2018
Closed
@thaJeztah

thaJeztah commented Feb 21, 2018

Copy link
Copy Markdown
Member

Thanks @AkihiroSuda - this does need a follow-up to update the documentation, for example, the docker service create reference doc mentions the accepted protocols, but there may be other locations (opened docker/docs#6039 for tracking that)

@thaJeztahthaJeztah mentioned this pull request Mar 6, 2018
Closed
silvin-lubecki added a commit to silvin-lubecki/cli that referenced this pull request Apr 12, 2018
@silvin-lubecki
Fixing issuedocker#278"No stack in docker help command"
c746dcf
The "kubernetes" and "swarm" annotations are removed as we don't want to hide stack command.
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
Vincent Demeester
Merge pull requestdocker#278from ishidawataru/sctp
53fd7b9
Support SCTP port mapping
Upstream-commit: 64f92fd
Component: cli
nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
@andrewhsu
Merge pull requestdocker#278from riyazdf/update-tests-for-17.11
19d1dd5
[17.11] update trust tests for 17.11
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83cpuguy83cpuguy83 left review comments
+2 more reviewers
@vdemeestervdemeestervdemeester approved these changes
@dnephindnephindnephin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

impact/changelogstatus/2-code-review

Projects

None yet

Milestone

18.03.0

Development

Successfully merging this pull request may close these issues.

9 participants

@ishidawataru@codecov-io@aaronlehmann@AkihiroSuda@dnephin@thaJeztah@vdemeester@cpuguy83@GordonTheTurtle