Fix all the dynamically generated code #547

mmlb · 2021-10-04T14:00:07Z

Lots of fixes to stuff that is dynamically generated along with a few extra bits here and there in related code to make things better.

Fixes the go generate calls
Changes protobuf/grpc stuff to use buf for both dependency management and building/calling protoc
Changes from goimports to gofumpt as we want this anyway
Build the tools in tools.go via the Makefile
Dropped protoc-gen-doc support as that seemed unused. (easy to bring back when we have a use for it)
Add a make target to check that the generated files are up to date
Prettier-ify the GHA yaml files too (not just *.yml files)
Update README/docs
Setup CI to ensure files are up to date

Mostly fixes for dynamically generated files.

I noticed that #541 had some protobuf format commits which I was not expecting being done separately. I went looking into it and noticed the mess we're in.

How Has This Been Tested?

CI

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

Will have better automation for messing with generated files.

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

codecov · 2021-10-04T19:18:51Z

Codecov Report

Merging #547 (8c6df7c) into main (e8019c9) will not change coverage.
The diff coverage is n/a.

❗ Current head 8c6df7c differs from pull request most recent head c06dde1. Consider uploading reports for the commit c06dde1 to get more accurate results

@@           Coverage Diff           @@
##             main     #547   +/-   ##
=======================================
  Coverage   34.76%   34.76%           
=======================================
  Files          44       44           
  Lines        3348     3348           
=======================================
  Hits         1164     1164           
  Misses       2085     2085           
  Partials       99       99

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 e8019c9...c06dde1. Read the comment docs.

mmlb · 2021-10-04T20:46:51Z

.github/workflows/ci.yaml

+      - name: make verify
+        run: make verify


Hey @tstromberg I modified this so it checks more than just lint steps. I think its in line with the idea behind lint-install's lint.

mmlb · 2021-10-04T20:48:17Z

Makefile

 all: cli server worker ## Build all binaries for host OS and CPU

 -include rules.mk
+-include lint.mk


I moved the lint-install managed stuff here as I'd like to keep the Makefile as simplish as possible... lesson learned from osie's ;)

mmlb · 2021-10-04T20:49:50Z

rules.mk

-grpc/gen_doc:
-	protoc \
-		-I./protos \
-		-I./protos/third_party \
-		--doc_out=./doc \
-		--doc_opt=html,index.html \
-		protos/hardware/*.proto protos/template/*.proto protos/workflow/*.proto


@displague I got rid of this because I couldn't find where it used. I don't think it is right? I'd love to use it but am not sure how we'd integrate the generated files (html or even markdown...)

I couldn't find anywhere it was used. We could potentially have a reference section in the docs with API schemas. When we are using K8s CRDs for storage, a section in the docs site might be especially helpful.

For that I'd expect the docs repo to clone and be the one calling protoc-gen-doc right?

Yes. K8s does something similar with submodules (not that we should do the same, just a reference point). At the point we want autogenerated API docs, we can add back some doc gen make targets

tstromberg · 2021-10-06T16:37:13Z

.github/workflows/ci.yaml

+        uses: actions/setup-go@v2
+        with:
+          go-version: "1.14.6"
+      - name: gofumpt


The golangci-lint config already includes gofumpt.

ah I thought lint-install did that and I didn't find it. will drop.

its been dropped

micahhausler · 2021-10-08T14:10:10Z

rules.mk

-grpc/gen_doc:
-	protoc \
-		-I./protos \
-		-I./protos/third_party \
-		--doc_out=./doc \
-		--doc_opt=html,index.html \
-		protos/hardware/*.proto protos/template/*.proto protos/workflow/*.proto


I couldn't find anywhere it was used. We could potentially have a reference section in the docs with API schemas. When we are using K8s CRDs for storage, a section in the docs site might be especially helpful.

micahhausler · 2021-10-08T14:11:56Z

rules.mk

+export PATH
+endif
+
+toolsBins := $(addprefix bin/,$(notdir $(shell awk -F'"' '/^\s*_/ {print $$2}' tools.go)))


Can this directory be used for other things too? I'm thinking about #542

Yep, if you update tools.go with the ref to the cmd you can update your makefile bits to depend on bin/$name and it'll all work out. So far I'm digging this pattern quite a bit over explicitly installing tools via make.

micahhausler · 2021-10-08T14:14:48Z

rules.mk

+
+.PHONY: pbfiles
+pbfiles: buf.gen.yaml buf.lock $(shell git ls-files 'protos/*/*.proto') $(toolsBins)
+	buf generate


Will this download the imported google protobuf definitions? do those need to be added to .gitignore?

Yep it does download them, but into a buf managed cache dir. The files never land into somewhere git sees.

Lets keep the Makefile easy to read. Signed-off-by: Manuel Mendez <[email protected]>

We already did .yml, but missed .yaml extension. Signed-off-by: Manuel Mendez <[email protected]>

gofumpt is goimports++. Signed-off-by: Manuel Mendez <[email protected]>

Also sets up the PATH so the binaries will be used by other steps. Signed-off-by: Manuel Mendez <[email protected]>

Signed-off-by: Manuel Mendez <[email protected]>

Its not used at the moment, so lets get rid of it until we have a use. Signed-off-by: Manuel Mendez <[email protected]>

Signed-off-by: Manuel Mendez <[email protected]>

The old code was just plain broken and did not generate anything. Signed-off-by: Manuel Mendez <[email protected]>

Signed-off-by: Manuel Mendez <[email protected]>

Makes more sense for a phony target, no need for the subdir in it. Also add a dep on bin/moq for correctness. Signed-off-by: Manuel Mendez <[email protected]>

Signed-off-by: Manuel Mendez <[email protected]>

Going off of the go grpc example from buf [tour]. tour: https://docs.buf.build/tour/generate-go-code/ Signed-off-by: Manuel Mendez <[email protected]>

Signed-off-by: Manuel Mendez <[email protected]>

I don't want to wire up the buf based generation as a dependendency of the binary targets as its almost always a waste of time. I'd rather be able to build quickly and leave CI and/or explicit check to catch the stale files. Signed-off-by: Manuel Mendez <[email protected]>

buf does it all, manages dependencies and invokes protoc. No need for 2 separate tools. Signed-off-by: Manuel Mendez <[email protected]>

Verify will no verify that generated files are up to date, along with its previous checks of lint free and formatted correctly. Signed-off-by: Manuel Mendez <[email protected]>

This way we check more than just linting and don't need to have one job for each and every check. Signed-off-by: Manuel Mendez <[email protected]>

Signed-off-by: Manuel Mendez <[email protected]>

mmlb force-pushed the fix-all-the-generators branch from e2f1eef to da146ba Compare October 4, 2021 19:09

mmlb force-pushed the fix-all-the-generators branch from da146ba to ef240d4 Compare October 4, 2021 20:45

mmlb marked this pull request as ready for review October 4, 2021 20:45

mmlb commented Oct 4, 2021

View reviewed changes

mmlb force-pushed the fix-all-the-generators branch from ef240d4 to b6250b3 Compare October 4, 2021 20:52

mmlb requested review from tstromberg and displague October 6, 2021 15:35

tstromberg suggested changes Oct 6, 2021

View reviewed changes

mmlb force-pushed the fix-all-the-generators branch from b6250b3 to ca2166e Compare October 6, 2021 18:07

mmlb requested a review from tstromberg October 6, 2021 18:07

mmlb force-pushed the fix-all-the-generators branch from ca2166e to b4465b6 Compare October 6, 2021 18:17

micahhausler reviewed Oct 8, 2021

View reviewed changes

mmlb added 17 commits October 8, 2021 15:19

Move lint-install stuff into lint.mk

8afcd09

Lets keep the Makefile easy to read. Signed-off-by: Manuel Mendez <[email protected]>

prettier-ify *.yaml files too

fe55fb8

We already did .yml, but missed .yaml extension. Signed-off-by: Manuel Mendez <[email protected]>

Replace goimports with gofumpt

8f06198

gofumpt is goimports++. Signed-off-by: Manuel Mendez <[email protected]>

make: Add targets to build tools pinned in tools.go

2357a89

Also sets up the PATH so the binaries will be used by other steps. Signed-off-by: Manuel Mendez <[email protected]>

make: Change the default target to help

00a1897

Signed-off-by: Manuel Mendez <[email protected]>

make: Ensure all phony targets are marked as such.

420b1ac

Signed-off-by: Manuel Mendez <[email protected]>

Drop protoc-gen-doc from nix and make

f566a30

Its not used at the moment, so lets get rid of it until we have a use. Signed-off-by: Manuel Mendez <[email protected]>

tools: add github.com/matryer/moq

af19855

Signed-off-by: Manuel Mendez <[email protected]>

make: Fix go generate call in protos/gen_mock

ddea0e8

The old code was just plain broken and did not generate anything. Signed-off-by: Manuel Mendez <[email protected]>

Regenerate protobuf mocks

db0d098

Signed-off-by: Manuel Mendez <[email protected]>

make: rename protos/gen_mock -> protomocks

8356d6c

Makes more sense for a phony target, no need for the subdir in it. Also add a dep on bin/moq for correctness. Signed-off-by: Manuel Mendez <[email protected]>

make: Add target for generated files

69ddd76

Signed-off-by: Manuel Mendez <[email protected]>

make: Add check-protomocks target

c7c005d

Signed-off-by: Manuel Mendez <[email protected]>

make: Add target to verify generated files are not stale

05c622a

Signed-off-by: Manuel Mendez <[email protected]>

tools: Add buf

1b94c39

Signed-off-by: Manuel Mendez <[email protected]>

Add buf files

abfe05a

Going off of the go grpc example from buf [tour]. tour: https://docs.buf.build/tour/generate-go-code/ Signed-off-by: Manuel Mendez <[email protected]>

make: Add target to generate protobuf files using buf

9542416

Signed-off-by: Manuel Mendez <[email protected]>

mmlb added 6 commits October 8, 2021 15:19

Regenerate protobuf based files

ca405c0

Signed-off-by: Manuel Mendez <[email protected]>

Get rid of non-buf protobuf stuff

3113970

buf does it all, manages dependencies and invokes protoc. No need for 2 separate tools. Signed-off-by: Manuel Mendez <[email protected]>

make: Add dependency on check-generated to verify target

7a78913

Verify will no verify that generated files are up to date, along with its previous checks of lint free and formatted correctly. Signed-off-by: Manuel Mendez <[email protected]>

ci: Change lint job to verify

6e5abe4

This way we check more than just linting and don't need to have one job for each and every check. Signed-off-by: Manuel Mendez <[email protected]>

docs: Update to mention buf replacing protoc.sh

c06dde1

Signed-off-by: Manuel Mendez <[email protected]>

mmlb force-pushed the fix-all-the-generators branch from b4465b6 to c06dde1 Compare October 8, 2021 19:20

tstromberg approved these changes Oct 13, 2021

View reviewed changes

mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Oct 13, 2021

mmlb removed the request for review from displague October 13, 2021 20:38

mergify bot merged commit 96fd7ce into tinkerbell:main Oct 13, 2021

mmlb deleted the fix-all-the-generators branch October 13, 2021 20:39

displague added this to the 0.7.0 milestone Aug 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix all the dynamically generated code #547

Fix all the dynamically generated code #547

mmlb commented Oct 4, 2021 •

edited

Loading

codecov bot commented Oct 4, 2021 •

edited

Loading

mmlb Oct 4, 2021

mmlb Oct 4, 2021

mmlb Oct 4, 2021

micahhausler Oct 8, 2021

mmlb Oct 8, 2021

micahhausler Oct 8, 2021 •

edited

Loading

mmlb Oct 8, 2021

tstromberg Oct 6, 2021

mmlb Oct 6, 2021

mmlb Oct 6, 2021

micahhausler Oct 8, 2021

micahhausler Oct 8, 2021

mmlb Oct 8, 2021

micahhausler Oct 8, 2021

mmlb Oct 8, 2021

Fix all the dynamically generated code #547

Fix all the dynamically generated code #547

Conversation

mmlb commented Oct 4, 2021 • edited Loading

How Has This Been Tested?

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

Checklist:

codecov bot commented Oct 4, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micahhausler Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmlb commented Oct 4, 2021 •

edited

Loading

codecov bot commented Oct 4, 2021 •

edited

Loading

micahhausler Oct 8, 2021 •

edited

Loading