Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation to proto files #243

Merged
merged 4 commits into from
Sep 18, 2020
Merged

Conversation

displague
Copy link
Member

@displague displague commented Aug 7, 2020

Description

This PR:

  • makes protos/protoc.sh executable in environments without protoc (and c++) installed
  • docker provides a reproducible protoc environment for regenerating pb.go files (jaegertracing/protobuf, libprotoc 3.8.0)
  • adds documentation to the hardware.proto file

Why is this needed

This starts the effort needed for #219

Additional .proto file docs would be needed to close that issue.

How Has This Been Tested?

Let me know how I should test this.

I would like to include a make protos task.
Should this be run by CI to check for uncommitted drift?

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

@displague displague changed the title Protoc docs Add documentation to proto files Aug 7, 2020
@displague displague force-pushed the protoc-docs branch 2 times, most recently from 3126118 to aa26b98 Compare August 7, 2020 12:13
@gianarb
Copy link
Contributor

gianarb commented Aug 7, 2020

I like the end goal: documentation and easy to replicate. But this adds a dependency with Docker and I don't like that in the same way.

If this is something you would like to add-in CI we use nix and shell.nix to resolve dependencies in the ci-checks.sh can you follow the same pattern?

@displague
Copy link
Member Author

this adds a dependency with Docker and I don't like that in the same way.

What are your thoughts on taking this PR as-is for the documentation changes and the protoc.sh changes (when the existing tests pass)?

If protoc is already installed, it won't use Docker. The protoc.sh script generates the .pb.go files that should be included in each commit (that modifies .proto files), it needs to be run by the developer locally.

The protoc.sh script is not usable without protoc installed, which is complicated because you need to use a package manager, download and unzip, or compile it (requiring a c++ environment). This makes me ❤️ go get even more.

I haven't added any build time changes in this PR. We could make this a part of CI later.

Independent of that, we can create a make protos task, which could replace the protoc.sh script.

Then, as I see it, make protos will have to do one of four things:

  1. Require the expected version of protoc to be installed, with an additional requirement for the expected version of the proto includes from Google
  2. or install these tools for you (in a version and build path isolated way)
  3. or use an existing Docker image
  4. or use and maintain our own Docker image

1 and 3 are the most sustainable, 3 is the easiest on new contributors.

@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #243 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #243   +/-   ##
=======================================
  Coverage   22.90%   22.90%           
=======================================
  Files          15       15           
  Lines        1275     1275           
=======================================
  Hits          292      292           
  Misses        964      964           
  Partials       19       19           

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 7dec4d8...4146325. Read the comment docs.

@displague displague force-pushed the protoc-docs branch 3 times, most recently from 5dd4bcd to 7e69698 Compare August 29, 2020 01:13
@displague displague requested a review from kqdeng September 5, 2020 11:42
@kqdeng
Copy link
Contributor

kqdeng commented Sep 8, 2020

Small nitpick, can you run goimports so that the imports inside the pb.go files get grouped properly?

@mmlb
Copy link
Contributor

mmlb commented Sep 8, 2020

#283 is relevant to this, 6f757ec addresses @kdeng3849's feedback too.

Signed-off-by: Marques Johansson <marques@packet.com>
Signed-off-by: Marques Johansson <marques@packet.com>
Signed-off-by: Marques Johansson <marques@packet.com>
@displague
Copy link
Member Author

@kdeng3849 @mmlb thanks for the goimports addition!

This is rebased and ready for another look.

protos/protoc.sh Show resolved Hide resolved
protos/protoc.sh Show resolved Hide resolved
@gianarb gianarb requested a review from mmlb September 18, 2020 11:35
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Sep 18, 2020
@mergify mergify bot merged commit 93b9338 into tinkerbell:master Sep 18, 2020
@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants