Uh oh!
There was an error while loading. Please reload this page.
Add support for regenerating protocol buffers, and refresh .pb.go files - #2193
Conversation
@fcrisciani, @ctelfer: This should make regenerating protocol buffers more consistent. I'll try to come up with a nice |
ctelfer
left a comment
There was a problem hiding this comment.
Definitely a worthwhile thing to add and the output looks sane from a brief glance. Have you tried building moby with the resulting libnetwork. Best double-check would be to replace one of the moby's in a cluster with one w/ the updated libnetwork and then run some test load of services to make sure all nodes can still communicate cleanly w/ each other. (I can't imagine the PB mods will break ... but need to verify of course.)
I haven't tried building moby with these changes yet. Is there a convenient set of tests which particularly exercises libnetwork? |
1d15e95 to
ebc2fdaCompareSigned-off-by: Euan Harris <euan.harris@docker.com>
agent.pb.go is unchanged, but the files in networkdb and drivers are slightly different when regenerated using the current versions of protoc and gogoproto. This is probably because agent.pb.go was last regenerated quite recently, in February 2018, whereas networkdb.pb.go and overlay/overlay.pb.go were last changed in 2017, and windows/overlay/overlay.pb.go was last changed in 2016. Signed-off-by: Euan Harris <euan.harris@docker.com>
euanh
commented
Jun 22, 2018
I tried using the vendored version of gogoproto, but instead of |
| PB_FILES=$(PROTO_FILES:.proto=.pb.go) | ||
| %.pb.go: %.proto | ||
| ${docker} protoc -I=. -I=/go/src -I=/go/src/github.com/gogo/protobuf -I=/go/src/github.com/gogo/protobuf/protobuf --gogo_out=./ $< |
There was a problem hiding this comment.
don't not have to be in this PR, but would be great to fail if the PR while compiled by the CI has a diff with the file generated compared to the files coming from the PR itself. This way we can enforce that the PR contains all the proper information.
Also does it makes sense to have build target depend on the protobuf generation?
selansen
commented
Jun 26, 2018
LGTM |
euanh
commented
Jun 26, 2018
@fcrisciani@selansen@ctelfer I'm currently running the tests that @ctelfer suggested and tracking down a couple of failures. |
euanh
commented
Jun 27, 2018
I reproduced the same failures running a mixed-version cluster with moby master (without my changes) on the leader and 17.06 on two other nodes. Updating all nodes to moby master made them pass. Similarly, with my changes only on the leader I had a couple of failures; with my changes on all nodes the tests passed. |
Previously the build container did not provide
protocorprotoc-gen-gogo; now these are available in the container. There is also a make pattern rule to regenerate.pb.gofiles from.protofiles.I regenerated the
.pb.gofiles.agent.go.pbwas regenerated recently and does not change, however older.pb.gofiles change slightly when regenerated with newer tools.