-
Notifications
You must be signed in to change notification settings - Fork 40
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
refactor(evpn-gw): updates to support buf lint and generation #411
Conversation
Signed-off-by: Mark Sanders <marksanders194@gmail.com>
network/evpn-gw/Makefile
Outdated
rm -rf ./google | ||
rm -rf ./v1alpha1/{autogen.md,gen,google} | ||
mkdir -p ./v1alpha1/gen/{go,cpp,python,java} | ||
rm -rf ./v1alpha1/{autogen.md} |
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.
{}
are not needed since we have only one option autogen.md
Also, since it is a single file, we can just use rm -f
network/evpn-gw/Makefile
Outdated
@@ -3,22 +3,22 @@ | |||
# Copyright (c) 2022 Dell Inc, or its subsidiaries. | |||
# Copyright (C) 2023 Nordix Foundation. | |||
|
|||
all: | |||
all: buflint doc |
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 intentionally not run bufgen
target? Without bufgen
, the CI job does not actually check that we provided all generated artifacts
opi-api/.github/workflows/network.yml
Lines 42 to 48 in b990048
- name: Build protobufs | |
run: make | |
working-directory: network | |
- name: Check uncomitted auto generated protobufs | |
run: git diff --exit-code | |
working-directory: network |
I missed that point on the review of opinetcommon
as well...
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, I will add that into the steps currently. Eventually the generated files will be placed into a separate repository and autogenerated; but, for now adding the bufgen in the make should take care of the operation.
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.
@sandersms I guess you are doing the transition to the buf lint but you still need to generate the go/python files by yourself. Right ? When we move the apis to their own repo then the generation of the go/python files will happen automatically. So this is a prepatory step. Right ?
@@ -337,9 +337,9 @@ enum BPOperStatus { | |||
// BridgePortType reflects the different types of a Bridge Port | |||
enum BridgePortType { | |||
// "unknown" bridge port type | |||
UNKNOWN = 0; | |||
BRIDGE_PORT_TYPE_UNSPECIFIED = 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.
What is the reason that we have changed the ENUM here ? Does this follows the googles AIPs for ENUMs ?
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 - this follows the AIPs for ENUMs as part of the checking. It also requires the addition of the "BRIDGE_PORT_TYPE_" for all of the ENUMs to be unique as a check.
@mardim91 Yes, this is the preparatory step to get to the generation of the apis and I will be adding the buf gen to the full make to have the go/python files generated as part of this PR as noted in one of the earlier comments. We will be moving the apis that are generated to their own repo once we get all of the capabilities in place. |
Signed-off-by: Mark Sanders <marksanders194@gmail.com>
@@ -3,3 +3,11 @@ name: buf.build/opiproject/evpn-gw | |||
deps: | |||
- buf.build/googleapis/googleapis | |||
- buf.build/grpc-ecosystem/grpc-gateway | |||
- buf.build/opiproject/opinetcommon | |||
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.
let's also include COMMENTS
linter rules here
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 to latest addition.
let's also include COMMENTS linter rules here
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.
LGTM
Signed-off-by: Mark Sanders <marksanders194@gmail.com>
564d6d3
to
e0ec268
Compare
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.
LGTM
Transition evpn-gw to the buf lint and buf generate operation to allow for automation of the protobuf file generation.
Support for cpp and java are being removed at this time. Generation supports go and python. Support for other languages can be added as needed by the buf.gen.yaml file.
Be aware that the .proto files are moved up from the v1alpha1 directory to support the buf operation and to keep the version of the generated files in a separate (v1, v1alpha1, etc.) directory structure. This will allow for the future ability to autogenerate the files and place them in a separate repo location.