k8snetworkplumbingwg / ib-sriov-cni

InfiniBand SR-IOV CNI
Other
42 stars 27 forks source link

go mod vendor #33

Closed zshi-redhat closed 4 years ago

zshi-redhat commented 4 years ago

enable building from local vendor dependencies

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 116


Totals Coverage Status
Change from base Build 73: 0.0%
Covered Lines: 214
Relevant Lines: 501

💛 - Coveralls
adrianchiris commented 4 years ago

Hi, can you elaborate why is this required? Is it for building without internet connectivity?

zshi-redhat commented 4 years ago

Hi, can you elaborate why is this required? Is it for building without internet connectivity?

Yes, for disconnected build. This unfortunately is the case for OpenShift, where all the images are built in system which doesn't allow getting vendor dependencies from internet.

adrianchiris commented 4 years ago

So i assume the only change is the addition of the vendor dir right ? (its hard to see as there are many files in the commit)

The vendor approach has the following caveat in regards to package dependencies: looking at the following packages : A, B, C, C , C being an older version of package C if A->B->C and A->C when "->" marks a package dependency, if the API used by B from C is not compatible with C, compilation will fail.

Correct me if i am wrong in the above example.

Are you OK with maintaining this ?

zshi-redhat commented 4 years ago

So i assume the only change is the addition of the vendor dir right ? (its hard to see as there are many files in the commit)

Right

The vendor approach has the following caveat in regards to package dependencies: looking at the following packages : A, B, C, C , C being an older version of package C if A->B->C and A->C when "->" marks a package dependency, if the API used by B from C is not compatible with C, compilation will fail.

My understanding is both A and B will depend on the same version of C or C*

Correct me if i am wrong in the above example.

Are you OK with maintaining this ?

Sure. I just run go mod vendor to download vendor pkgs locally, without changing go.mod or go.sum.

zshi-redhat commented 4 years ago

How can we make sure that the vendor is aligned with go mod? I am afraid with mistakes in review that will approve the change with go mod but will not update the vendor. @zshi-redhat, how do you make sure it sync in other projects?

@moshe010 vendor will be updated when developer explicitly run go mod tidy && go mod vendor. In sriov-network-device-plugin, it has deps update entry in Makefile: https://github.com/intel/sriov-network-device-plugin/blob/master/Makefile#L143 So every time, when a new PR requires changing vendor dependency, we shall include those vendor pkg changes in a separate commit. but not all PRs requires vendor update.

moshe010 commented 4 years ago

How can we make sure that the vendor is aligned with go mod? I am afraid with mistakes in review that will approve the change with go mod but will not update the vendor. @zshi-redhat, how do you make sure it sync in other projects?

@moshe010 vendor will be updated when developer explicitly run go mod tidy && go mod vendor. In sriov-network-device-plugin, it has deps update entry in Makefile: https://github.com/intel/sriov-network-device-plugin/blob/master/Makefile#L143 So every time, when a new PR requires changing vendor dependency, we shall include those vendor pkg changes in a separate commit. but not all PRs requires vendor update.

I see so can you just add the Makefile change as well or you plan to do it in another PR?

zshi-redhat commented 4 years ago

How can we make sure that the vendor is aligned with go mod? I am afraid with mistakes in review that will approve the change with go mod but will not update the vendor. @zshi-redhat, how do you make sure it sync in other projects?

@moshe010 vendor will be updated when developer explicitly run go mod tidy && go mod vendor. In sriov-network-device-plugin, it has deps update entry in Makefile: https://github.com/intel/sriov-network-device-plugin/blob/master/Makefile#L143 So every time, when a new PR requires changing vendor dependency, we shall include those vendor pkg changes in a separate commit. but not all PRs requires vendor update.

I see so can you just add the Makefile change as well or you plan to do it in another PR?

added in a separate commit