-
Notifications
You must be signed in to change notification settings - Fork 41
refactor: transition to buf v2 for api generation #469
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
Conversation
Signed-off-by: Mark Sanders <[email protected]>
Signed-off-by: Mark Sanders <[email protected]>
Signed-off-by: Mark Sanders <[email protected]>
Signed-off-by: Mark Sanders <[email protected]>
Signed-off-by: Mark Sanders <[email protected]>
Signed-off-by: Mark Sanders <[email protected]>
Signed-off-by: Mark Sanders <[email protected]>
Signed-off-by: Mark Sanders <[email protected]>
Signed-off-by: Mark Sanders <[email protected]>
Signed-off-by: Mark Sanders <[email protected]>
Signed-off-by: Mark Sanders <[email protected]>
|
||
bufformat: | ||
# format the protobuf using buf industry standard | ||
buf format --diff -w |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we have formatting in the makefile:
Would it make sense to force formatting in the github action?
Should we add bufformat
to all
?
Can we also used containerized version here: bufbuild/buf:1.50.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put it in the makefile to allow for someone to do the format step and make the appropriate corrections. The format command will force the write of the changes with the -w. You can just do a --diff to see what the changes are. Currently there are files in the .proto format that have not been formatted completely proper, so that still needs to be done as part of the V2 updates.
For the buf format --diff -w I will look at the buf:1.50.0 version also. I have it locally, but it may also be valuable in the container view. Let me investigate that separately.
The CI for the buf will have a check for the buf format, once the existing files are all format updated. That will be enabled in the CI and will force the format adherence. See Issue #470
-v "${PWD}":/base \ | ||
-v "${PWD}":/out \ | ||
-w /out \ | ||
ghcr.io/sandersms/bufbuild-go-gen:latest generate --template /base/buf.gen.yaml -o ${APIVER} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a reminder: #414
Pls also re-check if we have go-gen in bufbuild/buf:1.50.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the code pieces are not in the buf:1.50.0 that are used for file generation. The other pieces have to be pulled in to properly create the build files (like grpc). The bufbuild-go-gen is updated to the bufbuild/buf:1.50.0. I did that prior to creating the ghcr.io version that is released.
# OPI API and Behavioral Model Group | ||
|
||
[](https://github.com/opiproject/opi-api/actions/workflows/linters.yml) | ||
[](https://github.com/opiproject/opi-api/actions/workflows/storage.yml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a badge for buf.yml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is in the next step of the Buf V2 transition. I opened Issue #471
# Allow same name used in request/response type for multiple RPCs | ||
- RPC_REQUEST_RESPONSE_UNIQUE | ||
- ENUM_ZERO_VALUE_SUFFIX | ||
# disabled comment checking - todo: enable and resolve these errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have an issue for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added additional issues for the Buf V2 transition. Issue #470
all: buflint bufgen | ||
|
||
buflint: | ||
docker run --rm -v "${PWD}":/out -w /out bufbuild/buf:1.50.0 lint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had google api linter as apilint
target. Would it make sense to add it here until we have aep plugin running as a replacement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new buf linter seems to be including the api linter. I opened Issue #472 to look at this.
I rushed getting this checked in to allow for the transition to the Buf V2 and the Buf CI processing. I will cover the reported comments from the issues opened in the list. |
Transition API generation to buf v2 support. This requires changes to
Once all of the protobuf files have been properly formatted and checked using "buf format", the CI check for format can be enabled.
The existing generated file locations in each of the folders (inventory, network, storage, security) will be maintained in order to keep the bridge prototypes in the repositories working. These will be transitioned over time and eventually removed as the bridge prototypes move to the new structure.