refactor(cloud): update to support buf lint and generation automation #413
Conversation
| @@ -2,22 +2,23 @@ | |||
| # Copyright (C) 2022 Intel Corporation | |||
| # Copyright (c) 2022 Dell Inc, or its subsidiaries. | |||
|
|
|||
| all: | |||
| all: buflint doc | |||
There was a problem hiding this comment.
Please see my comments to
#411
| @@ -879,10 +889,15 @@ message DeleteVnicRequest { | |||
|
|
|||
| // Update vnic request | |||
| message UpdateVnicRequest { | |||
| // Name of Vnic requested | |||
| string name = 1 [ | |||
There was a problem hiding this comment.
To be honest, these Update* changes look strange...
The resource (vnic) already contains name and adding it one more time is suspicious. I assume we can resolve it on the annotation level. Can you try to annotate UpdateVnic the following way
rpc UpdateVnic (UpdateVnicRequest) returns (Vnic) {
option (google.api.http) = {
patch: "/v1/{vnic.name=vnics/*}"
body: "vnic"
};
option (google.api.method_signature) = "vnic, update_mask";
}There was a problem hiding this comment.
Yes, much cleaner solution using the different annotate.
| @@ -1604,7 +1604,6 @@ message DeleteSubnetRequest { | |||
|
|
|||
| // Update subnet request | |||
| message UpdateSubnetRequest { | |||
| // updated subnet info | |||
There was a problem hiding this comment.
I think we should not delete the comment here.
That's a pity that we cannot enable COMMENTS linter rules, since many comments are missing in these proto files.
There was a problem hiding this comment.
I think we should not delete the comment here. That's a pity that we cannot enable
COMMENTSlinter rules, since many comments are missing in these proto files.
I will get that comment back in there is was deleted inadvertently. I will look at the "buf" setting to see if there is a "Comments" linter rule that can be enabled separately.
There was a problem hiding this comment.
Enabled buf COMMENTS linting rule in latest update with two comment checks disabled currently to be worked in later updates.
| @@ -354,7 +354,7 @@ message BGPNLRIPrefixStatus { | |||
| int32 flap_starttime = 19; | |||
| // If bgpNlriPrefixBest is 'true', then this field is set to 'routeIsBest'. | |||
| // Otherwise, it reports the stage in the decision process when the route was determined to be non-best. | |||
| BGPRsnNotBest reason_not_best = 20; | |||
| BGPRouteReason reason_not_best = 20; | |||
There was a problem hiding this comment.
we could consider renaming the member as well
There was a problem hiding this comment.
we could consider renaming the member as well
We could change it to route_reason = 20; as an option. The intent is to report the reason for not having the best route which really is the reason for the particular route selection where one of the reason's is "Route is best". I will let @jainvipin respond on the changing since the elements are from his contribution and his input on the enum and parameter should be considered on this one.
There was a problem hiding this comment.
@sandersms - all good; few minor comments inline... will approve after those fixes.
| @@ -106,7 +106,7 @@ message BGPPeerSpec { | |||
| // send extended community attributes to neighbor | |||
| bool send_ext_comm = 6; | |||
| // peer is a route reflector client | |||
| BGPPeerRRClient rr_client = 7; | |||
| BGPPeerRR rr_client = 7; | |||
There was a problem hiding this comment.
usualally RR Client is different than RR; not sure about the reason behind this name change, and if this was linting related or something done for improving brevity.
There was a problem hiding this comment.
I will add the Client back to the enum and the parameter. It was the linting checking on the enum definitions that resulted in the change.
| @@ -354,7 +354,7 @@ message BGPNLRIPrefixStatus { | |||
| int32 flap_starttime = 19; | |||
| // If bgpNlriPrefixBest is 'true', then this field is set to 'routeIsBest'. | |||
| // Otherwise, it reports the stage in the decision process when the route was determined to be non-best. | |||
| BGPRsnNotBest reason_not_best = 20; | |||
| BGPRouteReason reason_not_best = 20; | |||
There was a problem hiding this comment.
agree with @artek-koltun - we should change the field/member name as well to reflect its intent; thanks for fixing this.
Signed-off-by: Mark Sanders <marksanders194@gmail.com>
Signed-off-by: Mark Sanders <marksanders194@gmail.com>
sandersms
force-pushed
the
cloud-automate
branch
from
a905aec
to
1ae0252
Compare
Transition cloud 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.