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 CI step to build using protoc with fatal warnings #237

Conversation

tiyash-basu-frequenz
Copy link
Contributor

@tiyash-basu-frequenz tiyash-basu-frequenz commented Apr 12, 2024

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 tiyash-basu-frequenz self-assigned this Apr 12, 2024
@tiyash-basu-frequenz tiyash-basu-frequenz requested a review from a team as a code owner April 12, 2024 08:26
@tiyash-basu-frequenz tiyash-basu-frequenz marked this pull request as draft April 12, 2024 08:26
@github-actions github-actions bot added part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:protobuf Affects the protocol buffer definition files labels Apr 12, 2024
@github-actions github-actions bot added the part:docs Affects the documentation label Apr 12, 2024
The steps in the CI file from the step `nox` onwards miss some cases, like
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.

Signed-off-by: Tiyash Basu <tiyash.basu@frequenz.com>
@tiyash-basu-frequenz tiyash-basu-frequenz changed the title Add a step to the CI to build with protoc Add CI step to build using protoc with fatal warnings Apr 12, 2024
@tiyash-basu-frequenz tiyash-basu-frequenz marked this pull request as ready for review April 12, 2024 09:22
Copy link

@matthias-wende-frequenz matthias-wende-frequenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@thea-leake thea-leake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT!

@tiyash-basu-frequenz
Copy link
Contributor Author

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.

@tiyash-basu-frequenz tiyash-basu-frequenz merged commit db018fb into frequenz-floss:v0.x.x Apr 12, 2024
12 checks passed
@tiyash-basu-frequenz tiyash-basu-frequenz deleted the 228_unused_imports branch April 12, 2024 12:58
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment, but I'm also fine to keep it as it is.

.github/workflows/ci.yaml Show resolved Hide resolved
@llucax
Copy link
Contributor

llucax commented Apr 12, 2024

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 ™️ 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
Copy link
Contributor

llucax commented Apr 12, 2024

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
Copy link
Contributor

llucax commented Apr 12, 2024

So we don't forget:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:docs Affects the documentation part:protobuf Affects the protocol buffer definition files part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report unused imports in CI
5 participants