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

Add portmap plugin #338

Merged
merged 1 commit into from
Jun 6, 2017

Conversation

caseydavenport
Copy link
Member

@caseydavenport caseydavenport commented May 31, 2017

Description

Adds the portmap plugin from containernetworking/plugins#1 to the cni installer. Currently, there is no release of the portmap plugin so this PR needs to wait until there is one.

Todos

  • Include portmap plugin, or reference upstream release.
  • Documentation

@caseydavenport caseydavenport changed the title [WIP] Add portmap plugin Add portmap plugin Jun 5, 2017
@tomdee
Copy link
Contributor

tomdee commented Jun 5, 2017

I'm not super stoked about having that binary checked in, since it will live in the history forever. But otherwise, the change looks fine.

Copy link
Contributor

@tomdee tomdee left a comment

Choose a reason for hiding this comment

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

LGTM - but I don't like the binary

@tomdee
Copy link
Contributor

tomdee commented Jun 5, 2017

Actually - please also say (somewhere) exactly what git hash the binary is built from.

@gunjan5
Copy link
Contributor

gunjan5 commented Jun 5, 2017

What's wrong with leaving it as a Dockerhub image (with binary in it) until the plugin PR is merged so we don't have to upload the binary to the repo?

@caseydavenport
Copy link
Member Author

caseydavenport commented Jun 5, 2017

What's wrong with leaving it as a Dockerhub image (with binary in it) until the plugin PR is merged so we don't have to upload the binary to the repo?

We could do that. It would mean that building that version of the calico/cni container wouldn't be easily reproducible (but we could include the commit hash from which it was built to make that easier?)

@caseydavenport caseydavenport force-pushed the portmap-plugin branch 2 times, most recently from 5f548d1 to afd172d Compare June 6, 2017 00:27
@caseydavenport caseydavenport merged commit dc1d997 into projectcalico:master Jun 6, 2017
@caseydavenport caseydavenport deleted the portmap-plugin branch June 6, 2017 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants