search

contiv / ofnet

Ofnet is openflow networking library
Other
79 stars 73 forks source link

migrate to vendor #82

Closed unclejack closed 7 years ago

unclejack commented 7 years ago

This PR migrates ofnet to vendor.

This PR makes the following changes:

Most of these changes were made in one commit because godep was causing some issues with vendor.

dseevr commented 7 years ago

Changes look fine but the build is failing...

unclejack commented 7 years ago

The failures don't seem to be related to the libovsdb, netlink and netns. Reverting those doesn't fix the issue.

unclejack commented 7 years ago

@jojimt: Could you take a look at the logs when you have the time, please? I'm not sure why it's failing right now.

unclejack commented 7 years ago

I've reverted the change which was switching to contiv/centos73. This fixes the failure we were seeing in the tests.

I've opened an issue to track the TestPolicyAddDelete failure: #87.

This PR will allow us to bump the other deps (libovsdb, libOpenflow).

This PR is ready for review. @dseevr @jojimt PTAL

abhi commented 7 years ago

the last time we tried moving ofnet to vendoring (go1.6) there was dependency issue when ofnet was vendored into netplugin. Did you get a chance to test out netplugin with the vendoring ?

unclejack commented 7 years ago

@abhinandanpb: I'll try this out and post a reply. The Godeps workspace was broken without this PR. I can imagine that's what was breaking it before.

abhi commented 7 years ago

what was broken ? We have been using godeps forever with bunch of dependency brought in and out. We havent had any issues with godeps in ofnet.

unclejack commented 7 years ago

@abhinandanpb: How are you using godeps to update dependencies in ofnet? I get errors.

unclejack commented 7 years ago

@abhinandanpb: Please let me know if there's anything else you'd like me to do or look into.

unclejack commented 7 years ago

This PR is rebased on top of master.

I haven't changed the version of the Vagrant box. We have to address some failures with Go 1.7 in a separate PR.

The libOpenflow library is also not upgraded or modified in any way in this PR. That's to be done in a separate PR.

@DivyaVavili @gkvijay @rhim: We need this PR. PTAL

dvavili commented 7 years ago

Also, what is the recommended way of moving to vendor when it is used in other repositories? Will it even make a difference?

dseevr commented 7 years ago

@DivyaVavili it doesn't make a difference as to how the code is used under vendor... run this in netplugin and you'll see we still have one dependency using the legacy _workspace dir with our modern vendoring: find vendor -type d -name _workspace

unclejack commented 7 years ago

@DivyaVavili: There's nothing to check with netplugin in this case. ofnet itself is functioning properly and it's brought in with all of its dependencies. The difference is that we won't be running into errors any more when we try to bump dependencies.

We need to make progress and improve the code, but this is a major roadblock.

dseevr commented 7 years ago

The full netplugin CI suite will be run when netplugin's Godeps are updated to use the new version of this code.

We need to switch to vendor. It's been the ordained way to manage dependencies in Go projects for almost 1.5 years, and our code no longer compiles with any version of Go which does not support vendoring, and _workspace support is gone as of 1.8

We should merge this ASAP so that we can open the PR to update this dependency in netplugin itself

unclejack commented 7 years ago

I've tested this with netplugin. ofnet was bumped properly. There's some work left to be done when we bump this in netplugin itself. This work which needs to be done isn't something which should block this PR, though. It should be safe to proceed at this point.

dvavili commented 7 years ago

Thanks for checking this. LGTM with respect to this change. But when we are including it into contiv/netplugin, let's check it rigorously before merging in for the next major release so that we don't run into any dependency issues.