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

Support SCTP port mapping #278

Merged
merged 1 commit into from
Feb 21, 2018
Merged

Support SCTP port mapping #278

merged 1 commit into from
Feb 21, 2018

Conversation

ishidawataru
Copy link
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
Copy link

codecov-io commented Jul 3, 2017

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

@aaronlehmann
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Collaborator

rebased

@dnephin
Copy link
Contributor

dnephin commented Feb 2, 2018

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

@thaJeztah
Copy link
Member

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

@thaJeztah
Copy link
Member

moby/moby#33922 was merged

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

opts/port.go Outdated
@@ -49,7 +49,8 @@ func (p *PortOpt) Set(value string) error {

switch key {
case portOptProtocol:
if value != string(swarm.PortConfigProtocolTCP) && value != string(swarm.PortConfigProtocolUDP) {
// TODO: use swarm.PortConfigProtocolSCTP (vendor moby/moby#33922)
Copy link
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
Copy link
Member

Opened a vendor PR for moby/moby: #892

@AkihiroSuda
Copy link
Collaborator

Will rebase soon after #892 gets merged, thanks

Signed-off-by: Wataru Ishida <ishida.wataru@lab.ntt.co.jp>
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda
Copy link
Collaborator

done

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@thaJeztah
Copy link
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)

silvin-lubecki added a commit to silvin-lubecki/cli that referenced this pull request Apr 12, 2018
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>
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.

9 participants