search

k8snetworkplumbingwg / ovs-cni

Open vSwitch CNI plugin
Apache License 2.0
222 stars 70 forks source link

Bump protobuf v1.3.2 #195

Closed RamLavi closed 3 years ago

RamLavi commented 3 years ago

/release-note-none

RamLavi commented 3 years ago

running the test manually, I get this error: not sure how to tackle this, as tls.Dialer is defined by crypto/tls library..

# github.com/ovn-org/libovsdb/client
.gopath/pkg/mod/github.com/ovn-org/libovsdb@v0.6.1-0.20210820090057-e418ed547c02/client/client.go:132:14: undefined: tls.Dialer
phoracek commented 3 years ago

@RamLavi could there be a mismatch between the Golang versions that we and the dependency use? But it is hard to believe Golang would change name of something in the core library

RamLavi commented 3 years ago

@RamLavi could there be a mismatch between the Golang versions that we and the dependency use? But it is hard to believe Golang would change name of something in the core library

it does say note: module requires Go 1.16, but we are already set to go 1.16 also tried to go get github.com/ovn-org/libovsdb but it didn't help..

phoracek commented 3 years ago

After I called go mod vendor, make started giving me way more sensible messages:

# github.com/k8snetworkplumbingwg/ovs-cni/pkg/ovsdb
pkg/ovsdb/ovsdb.go:126:37: not enough arguments in call to ovsd.ovsClient.Transact
        have (...ovsdb.Operation)
        want (context.Context, ...ovsdb.Operation)
make: *** [Makefile:54: format] Error 2
RamLavi commented 3 years ago

After I called go mod vendor, make started giving me way more sensible messages:

# github.com/k8snetworkplumbingwg/ovs-cni/pkg/ovsdb
pkg/ovsdb/ovsdb.go:126:37: not enough arguments in call to ovsd.ovsClient.Transact
        have (...ovsdb.Operation)
        want (context.Context, ...ovsdb.Operation)
make: *** [Makefile:54: format] Error 2

I'm afraid I don't get the same results. Let separate the vendoring fix from the protobuf to avoid confusion. creating a new PR https://github.com/k8snetworkplumbingwg/ovs-cni/pull/196

After the issue is resolved we can continue on this PR with updating protobuf /hold

phoracek commented 3 years ago

How could you separate the two? In the PR you are changing the dependency in go.mod. With that you have to also update the vendoring directory, otherwise CI will complain:

go: inconsistent vendoring in /root/go/src/github.com/k8snetworkplumbingwg/ovs-cni:
...
    To sync the vendor directory, run:
        go mod vendor
make: *** [Makefile:67: test] Error 1

Maybe I'm missing something

RamLavi commented 3 years ago

How could you separate the two? In the PR you are changing the dependency in go.mod. With that you have to also update the vendoring directory, otherwise CI will complain:

go: inconsistent vendoring in /root/go/src/github.com/k8snetworkplumbingwg/ovs-cni:
...
  To sync the vendor directory, run:
      go mod vendor
make: *** [Makefile:67: test] Error 1

Maybe I'm missing something

well, the vendoring issue we fix on #196 is unrelated to bumping protobuf to v1.3.2, and appears on main branch as well. so might as well move to separate PRs..

RamLavi commented 3 years ago

@phoracek the PR is ready for review now

RamLavi commented 3 years ago

/hold cancel

kubevirt-bot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: phoracek, RamLavi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/k8snetworkplumbingwg/ovs-cni/blob/main/OWNERS)~~ [phoracek] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment