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 <mmendez@equinix.com>

We already did .yml, but missed .yaml extension. Signed-off-by: Manuel Mendez <mmendez@equinix.com>

gofumpt is goimports++. Signed-off-by: Manuel Mendez <mmendez@equinix.com>

Also sets up the PATH so the binaries will be used by other steps. Signed-off-by: Manuel Mendez <mmendez@equinix.com>