Show protos some love #345

mmlb · 2020-10-19T21:06:48Z

Description

This overhauls and fixes a bunch of warnings/nits in our protobufs. Also, I wasn't able to build the protobufs because of version mismatches and what not.

Why is this needed

I was not happy with the state of our protobuf build setup, managing protobuf dependencies and general protobuf lint/style issues.

Also,
Fixes: #319

How Has This Been Tested?

Builds and go test ./... works.

How are existing users impacted? What migration steps/scripts do we need?

Custom clients using our protobuf files will need to be updated, I don't know of any such clients.

codecov · 2020-10-19T21:08:57Z

Codecov Report

Merging #345 into master will not change coverage.
The diff coverage is 65.00%.

@@           Coverage Diff           @@
##           master     #345   +/-   ##
=======================================
  Coverage   23.79%   23.79%           
=======================================
  Files          14       14           
  Lines        1244     1244           
=======================================
  Hits          296      296           
  Misses        928      928           
  Partials       20       20

Impacted Files	Coverage Δ
grpc-server/grpc_server.go	`0.00% <0.00%> (ø)`
grpc-server/template.go	`19.19% <ø> (ø)`
http-server/http_handlers.go	`6.68% <0.00%> (ø)`
grpc-server/workflow.go	`20.60% <25.00%> (ø)`
grpc-server/tinkerbell.go	`92.12% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e7d539...87ab907. Read the comment docs.

jacobweinstock

you have a buf.yaml file in here, but I didn't see buf being run anywhere? did i miss it?

displague · 2020-10-20T20:05:03Z

tools.go

@@ -0,0 +1,12 @@
+// +build tools


What would the relationship be between this file and go.mod? Is this adding versioned dependencies that would be pruned by go mod tidy when not making a tools build?

Could tools.go live in the protoc/ directory with an isolated go.mod in that directory? (go list -m all could be used to list the dependencies if that were the case.)

We run into the problem that users will have the dependencies already installed, but at the wrong versions of those dependencies, resulting in proto file jitter depending on the committer.

The biggest potential benefit of this change is that the protobuf tooling version used to create the generated files would match the ones that are used in other parts of this project. If that is the goal, then the containerized protoc.sh approach should be removed.

The proto builds must be repeatable. I would like to see a protoc build added to the Github actions to look for proto file diffs that were not incorporated in a commit.

If controlling the local version of the proto tools is problematic, I would recommend that we use our own Dockerfile to control the tooling used in the proto file builds.

What would the relationship be between this file and go.mod? Is this adding versioned dependencies that would be pruned by go mod tidy when not making a tools build?

This file is used in harmony with go.mod. This is coming from one of the options in the Go Wiki, https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module.

Could tools.go live in the protoc/ directory with an isolated go.mod in that directory? (go list -m all could be used to list the dependencies if that were the case.)

I'd prefer not to do that. For one it seems weird to define a mod in a sub directory since mods are meant to be at a repo root right? Having these deps in the top-level mod also ensures they are always being used.

We run into the problem that users will have the dependencies already installed, but at the wrong versions of those dependencies, resulting in proto file jitter depending on the committer.

This takes care of that since we'd be using the pinned versions of the tools from the go.mod file.

The biggest potential benefit of this change is that the protobuf tooling version used to create the generated files would match the ones that are used in other parts of this project. If that is the goal, then the containerized protoc.sh approach should be removed.

I meant to do this, but forgot. This PR still doesn't deal with protoc versions/installations. I deal with that with shell.nix personally, the build-in-docker approach in protoc.sh also helps there too.

The proto builds must be repeatable. I would like to see a protoc build added to the Github actions to look for proto file diffs that were not incorporated in a commit.

Can do.

If controlling the local version of the proto tools is problematic, I would recommend that we use our own Dockerfile to control the tooling used in the proto file builds.

I don't particularly like leaning so much on docker for all dev tooling, though I think the happy medium is a nix docker image that reads the shell.nix file and uses those versions :D.

mmlb · 2020-10-20T20:15:06Z

you have a buf.yaml file in here, but I didn't see buf being run anywhere? did i miss it?

Not in an automated fashion. I used it locally to run some lint checks. I'm not personally ready to introduce that yet. I think we'd also need a layout change to make buf happy. I'll most likely do this in a separate PR.

mmlb · 2020-10-20T20:47:37Z

The proto builds must be repeatable. I would like to see a protoc build added to the Github actions to look for proto file diffs that were not incorporated in a commit.

Can do.

actually @displague I'd like to do that in a follow up PR. This one doesn't make things any worse wrt CI and is already pretty big.

mmlb · 2020-10-20T21:31:17Z

go.mod

 	gotest.tools v2.2.0+incompatible // indirect
 )
+
+replace github.com/stormcat24/protodep => github.com/ackintosh/protodep v0.0.0-20200728152107-abf8eb579d6c


replaced with stormcat24/protodep#49, otherwise I was only able to make it work with a bogus -i ssh identity file

gianarb · 2020-10-26T08:06:09Z

@mmlb if you are experimenting with buf locally can you remove it from the vcs? as you said, you can do it all together in its own PR.

gianarb · 2020-10-27T14:01:31Z

Overall I like what I see and I think we have to merge this PR pretty soon, it is big and will require a lot of work in order to keep it up to date with the codebase and I would like to avoid that.

@mmlb I think you should write a note in CONTRIBUTORS.md about how we can generate proto and how the tools package works. Just to have some sort of documentation.

Thanks

mmlb · 2020-10-27T14:05:02Z

Overall I like what I see and I think we have to merge this PR pretty soon, it is big and will require a lot of work in order to keep it up to date with the codebase and I would like to avoid that.

Yep, I have one more thing to check and make sure isn't broken as it failed for me in some tests yesterday.

@mmlb I think you should write a note in CONTRIBUTORS.md about how we can generate proto and how the tools package works. Just to have some sort of documentation.

I'll do that in a follow up PR to avoid growing this one more.

mmlb · 2020-10-27T14:29:45Z

@displague @jacobweinstock @gianarb PTAL, I've rebased on top of master to pull the goimports commit out of here. I also got rid of the buf.yml and dep in tools.go.

gianarb · 2020-10-27T14:32:23Z

I'll do that in a follow up PR to avoid growing this one more.

Follow up PR for documentation scares me so much xD Often they never arrive! xD

mmlb · 2020-10-27T14:33:18Z

:D especially with me :D. Ok I'll whip something up then.

mmlb · 2020-10-27T14:33:39Z

hmm we don't even have a CONTRIBUTING.md doc, grabbing from .github.

This way matches the docs and is easier to reason about. Signed-off-by: Manuel Mendez <manny@packet.com>

Using setup from https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module Signed-off-by: Manuel Mendez <manny@packet.com>

gnused gets us `sed` which will be used in protoc.sh in a following commit. protobuf gets us protoc. Signed-off-by: Manuel Mendez <manny@packet.com>

This way we can control the versions of all the tools. Signed-off-by: Manuel Mendez <manny@packet.com>

Its just easier this way to always expect to be at the root. Signed-off-by: Manuel Mendez <manny@packet.com>

Signed-off-by: Manuel Mendez <manny@packet.com>

Using the jaegertracing container method would make it hard to have reproducible builds as we can't use the pinned version of the protoc plugins we are using. Using protodeb and pinned versions in tools.go all we need is awk, go, and protoc. Signed-off-by: Manuel Mendez <manny@packet.com>

Signed-off-by: Manuel Mendez <manny@packet.com>

https://developers.google.com/protocol-buffers/docs/style Signed-off-by: Manuel Mendez <manny@packet.com>

Signed-off-by: Manuel Mendez <manny@packet.com>

It was mostly a 1:1 of enum State anyway. Signed-off-by: Manuel Mendez <manny@packet.com>

Signed-off-by: Manuel Mendez <manny@packet.com>

protoc-gen-go was throwing a warning: > Generating protos/workflow/workflow.pb.go... > 2020/10/19 16:57:11 WARNING: Deprecated use of 'go_package' option without a full import path in "workflow/workflow.proto", please specify: > option go_package = "workflow"; > A future release of protoc-gen-go will require the import path be specified. > See https://developers.google.com/protocol-buffers/docs/reference/go-generated#package for more information. Signed-off-by: Manuel Mendez <manny@packet.com>

Taken from boots and added info for building the protobuf generated files. Signed-off-by: Manuel Mendez <manny@packet.com>

mmlb · 2020-10-27T15:58:05Z

Alright @gianarb I threw in CONTRIBUTING.md over from boots and added a bit about protobuf files: https://github.com/mmlb/tink/blob/show-protos-some-love/CONTRIBUTING.md

@displague I dropped the use of jaegertracing/protobuf too

Signed-off-by: Kelly Deng <kelly@packet.com> ## Description  Following the merge of tinkerbell/tink#345, the `github/tinkerbell/tink` dependency in `go.mod` needs to be updated to point to the latest master. ## Why is this needed  The PR tinkerbell/tink#345 updated the protobuf files and as a result, hegel (still importing an older version of tink) can no longer properly communicate with the latest version of tink. Fixes: # ## How Has This Been Tested?    * Tests pass * Tested manually ## How are existing users impacted? What migration steps/scripts do we need?   ## Checklist: I have: - [ ] updated the documentation and/or roadmap (if required) - [ ] added unit or e2e tests - [ ] provided instructions on how to upgrade

Signed-off-by: Kelly Deng <kelly@packet.com> ## Description  Following the merge of tinkerbell/tink#345, the `github/tinkerbell/tink` dependency in `go.mod` needs to be updated to point to the latest master. ## Why is this needed  The PR tinkerbell/tink#345 updated the protobuf files and as a result, boots (still importing an older version of tink) can no longer properly communicate with the latest version of tink. Fixes: # ## How Has This Been Tested?    * Tests pass * Tested manually ## How are existing users impacted? What migration steps/scripts do we need?   ## Checklist: I have: - [ ] updated the documentation and/or roadmap (if required) - [ ] added unit or e2e tests - [ ] provided instructions on how to upgrade

mmlb force-pushed the show-protos-some-love branch from e5a4db3 to 47c3e87 Compare October 19, 2020 21:28

mmlb requested a review from jacobweinstock October 20, 2020 14:04

mmlb force-pushed the show-protos-some-love branch from 47c3e87 to 755d06d Compare October 20, 2020 14:04

mmlb requested review from kqdeng and gauravgahlot October 20, 2020 14:06

mmlb force-pushed the show-protos-some-love branch from 755d06d to a28a160 Compare October 20, 2020 14:07

mmlb mentioned this pull request Oct 20, 2020

Consistently name client interfaces #319

Closed

mmlb force-pushed the show-protos-some-love branch from a28a160 to 57732cf Compare October 20, 2020 16:51

jacobweinstock previously approved these changes Oct 20, 2020

View reviewed changes

displague reviewed Oct 20, 2020

View reviewed changes

mmlb dismissed jacobweinstock’s stale review via 38aa4ff October 20, 2020 21:29

mmlb force-pushed the show-protos-some-love branch from 57732cf to 38aa4ff Compare October 20, 2020 21:29

mmlb commented Oct 20, 2020

View reviewed changes

mmlb force-pushed the show-protos-some-love branch from 38aa4ff to 9bc6003 Compare October 27, 2020 14:27

mmlb removed request for gauravgahlot and kqdeng October 27, 2020 14:28

mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Oct 27, 2020

mmlb force-pushed the show-protos-some-love branch from 9bc6003 to f165a16 Compare October 27, 2020 15:50

mmlb added 2 commits October 27, 2020 11:56

protoc.sh: split out protoc plugin opts and out params

24cc0d3

This way matches the docs and is easier to reason about. Signed-off-by: Manuel Mendez <manny@packet.com>

Track tool dependencies

7bec26e

Using setup from https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module Signed-off-by: Manuel Mendez <manny@packet.com>

mmlb added 14 commits October 27, 2020 11:56

shell.nix: add gnused protobuf packages

d760c2a

gnused gets us `sed` which will be used in protoc.sh in a following commit. protobuf gets us protoc. Signed-off-by: Manuel Mendez <manny@packet.com>

Use protoc plugins from tools.go

55c2a27

This way we can control the versions of all the tools. Signed-off-by: Manuel Mendez <manny@packet.com>

Rework protoc.sh to expect to be run from repo root dir

815c719

Its just easier this way to always expect to be at the root. Signed-off-by: Manuel Mendez <manny@packet.com>

Use protodep to manage protobuf dependencies

b682453

Signed-off-by: Manuel Mendez <manny@packet.com>

Regenerate protobuf files

2df63b2

Signed-off-by: Manuel Mendez <manny@packet.com>

Reindent proto files to match Google Style Guide

8a98a48

https://developers.google.com/protocol-buffers/docs/style Signed-off-by: Manuel Mendez <manny@packet.com>

Fix protobuf field names to match Google Style Guide

6a36c85

Signed-off-by: Manuel Mendez <manny@packet.com>

Fix protobuf enums to match Google Style Guide

72c1c6c

Signed-off-by: Manuel Mendez <manny@packet.com>

Drop ActionState enum

08dd2a2

It was mostly a 1:1 of enum State anyway. Signed-off-by: Manuel Mendez <manny@packet.com>

Rename WorkflowSvc -> WorkflowService to match Google Style Guide

cff5ef2

Signed-off-by: Manuel Mendez <manny@packet.com>

Rename Template -> TemplateService to match Google Style Guide

d095044

Signed-off-by: Manuel Mendez <manny@packet.com>

Add CONTRIBUTING.md

87ab907

Taken from boots and added info for building the protobuf generated files. Signed-off-by: Manuel Mendez <manny@packet.com>

mmlb force-pushed the show-protos-some-love branch from f165a16 to 87ab907 Compare October 27, 2020 15:56

gianarb approved these changes Oct 27, 2020

View reviewed changes

mergify bot merged commit 0b477fd into tinkerbell:master Oct 27, 2020

This was referenced Oct 28, 2020

Update dependencies (tink) tinkerbell/smee#98

Merged

update dependencies (tink) tinkerbell/hegel#48

Merged

mmlb deleted the show-protos-some-love branch November 2, 2020 20:10

mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show protos some love #345

Show protos some love #345

mmlb commented Oct 19, 2020 •

edited

Loading

codecov bot commented Oct 19, 2020 •

edited

Loading

jacobweinstock left a comment

displague Oct 20, 2020

mmlb Oct 20, 2020 •

edited

Loading

mmlb commented Oct 20, 2020 •

edited

Loading

mmlb commented Oct 20, 2020

mmlb Oct 20, 2020

gianarb commented Oct 26, 2020

gianarb commented Oct 27, 2020

mmlb commented Oct 27, 2020

mmlb commented Oct 27, 2020 •

edited

Loading

gianarb commented Oct 27, 2020

mmlb commented Oct 27, 2020

mmlb commented Oct 27, 2020

mmlb commented Oct 27, 2020

Show protos some love #345

Show protos some love #345

Conversation

mmlb commented Oct 19, 2020 • edited Loading

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

codecov bot commented Oct 19, 2020 • edited Loading

Codecov Report

jacobweinstock left a comment

Choose a reason for hiding this comment

displague Oct 20, 2020

Choose a reason for hiding this comment

mmlb Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

mmlb commented Oct 20, 2020 • edited Loading

mmlb commented Oct 20, 2020

mmlb Oct 20, 2020

Choose a reason for hiding this comment

gianarb commented Oct 26, 2020

gianarb commented Oct 27, 2020

mmlb commented Oct 27, 2020

mmlb commented Oct 27, 2020 • edited Loading

gianarb commented Oct 27, 2020

mmlb commented Oct 27, 2020

mmlb commented Oct 27, 2020

mmlb commented Oct 27, 2020

mmlb commented Oct 19, 2020 •

edited

Loading

codecov bot commented Oct 19, 2020 •

edited

Loading

mmlb Oct 20, 2020 •

edited

Loading

mmlb commented Oct 20, 2020 •

edited

Loading

mmlb commented Oct 27, 2020 •

edited

Loading