search

frequenz-floss / frequenz-api-microgrid

gRPC+protobuf specification and Python bindings for the Frequenz Microgrid API
https://frequenz-floss.github.io/frequenz-api-microgrid/
MIT License
6 stars 6 forks source link

Add CI step to build using `protoc` with fatal warnings #237

Closed tiyash-basu-frequenz closed 7 months ago

tiyash-basu-frequenz commented 7 months ago

The steps in the CI file from the step nox onwards miss a few cases, like reporting unused imports. An additional step with protoc takes care of this. This step generates the protoc files directly using protoc, and treats warnings as fatal.

Closes #228

tiyash-basu-frequenz commented 7 months ago

Looks good but I wonder if that checks should be moved to nox and repo-config. @llucax Opinions?

Right, this should be added to repo-config at some point. Also, the action for setting up protoc has to be extracted into a separate action.

llucax commented 7 months ago

Looks good but I wonder if that checks should be moved to nox and repo-config. @llucax Opinions?

yes, but no, but yes

repo-config is needing some love, we are not being able to catch-up with the pace of new repo types (like api clients), and I want to split it in smaller repos (one for generating protobuf, one with the tools for mkdocs, one with just the cookicutter templates, etc.). We want to remove the python code generation too soon :tm: from "api" repos, so there is a lot of work to do on that side too. So I think we can wait for all that restructuring before pushing this to repo-config. Eventually I think we'll need to do a sprint to re-repo-configy all repositories.

llucax commented 7 months ago

Right, this should be added to repo-config at some point. Also, the action for setting up protoc has to be extracted into a separate action.

Yeah, that too, I started the process of splitting the big ci.yaml workflow in repo-config in smaller GitHub actions! But didn't have the time to update the cookiecutter templates to use those yet (which are also probably not done).

llucax commented 7 months ago

So we don't forget: