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

pkg/ns: refactored so that builds succeed on non-linux platforms #375

Merged
merged 4 commits into from
Feb 27, 2017

Conversation

aaithal
Copy link
Contributor

@aaithal aaithal commented Feb 20, 2017

Summary

moved functions that depend on linux packages (sys/unix) into a separate file
and added nop methods with a build tag for non-linux platforms in a new file.

Testing Done

./test ran successfully, apart from the macvlan Operations suite. I tested this on Amazon Linux and there's no support for macvlan on that platform. The failure log is pasted next:

• Failure [0.034 seconds]
macvlan Operations
/home/ec2-user/workspace/src/github.com/containernetworking/cni/gopath/src/github.com/containernetworking/cni/plugins/main/macvlan/macvlan_test.go:192
  creates an macvlan link in a non-default namespace [It]
  /home/ec2-user/workspace/src/github.com/containernetworking/cni/gopath/src/github.com/containernetworking/cni/plugins/main/macvlan/macvlan_test.go:102

  Expected error:
      <*errors.errorString | 0xc42014a690>: {
          s: "failed to create macvlan: operation not supported",
      }
      failed to create macvlan: operation not supported
  not to have occurred

  /home/ec2-user/workspace/src/github.com/containernetworking/cni/gopath/src/github.com/containernetworking/cni/plugins/main/macvlan/macvlan_test.go:87

moved functions that depend on linux packages (sys/unix) into a separate file
and added nop methods with a build tag for non-linux platforms in a new file.
@dcbw
Copy link
Member

dcbw commented Feb 23, 2017

Should ns_linux.go have a +build linux in it?

@squeed
Copy link
Member

squeed commented Feb 23, 2017

Nah, ending a file with _$GOOS is an implicit build tag.


package ns

func getCurrentThreadNetNSPath() string {
Copy link
Member

Choose a reason for hiding this comment

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

This makes me a bit uncomfortable: this is used in ns.Do(). I know that it currently won't cause problems, but...
Could you refactor Do() to use GetCurrentNS(), move GetCurrentNS() to the per-os files, and move getCurrentThreadNetNSPath to ns_linux.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure. That's cleaner than how I did it.

Make GetCurrentNS platform specific instead of getCurrentThreadNetNSPath
moved functions that depend on linux packages into a separate file and added
nop methods with a build tag for non-linux platforms in a new file.
@aaithal
Copy link
Contributor Author

aaithal commented Feb 23, 2017

@squeed I have updated the PR to address your comments, could you please take another look?

@dcbw as per @squeed's comments, the implicit tag should take care of it.

@squeed
Copy link
Member

squeed commented Feb 27, 2017

Wow, I'm surprised that netlink builds on non-linux :-).

The code changes LGTM. If anyone wants the commits to be squashed, that'd be reasonable.

@dcbw
Copy link
Member

dcbw commented Feb 27, 2017

Updates LGTM.

@dcbw dcbw merged commit 94e02e0 into containernetworking:master Feb 27, 2017
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