🧹Remove unreferenced protobuf messages #433
Conversation
control/cli.py
Outdated
| @@ -26,8 +26,6 @@ | |||
| from .utils import GatewayUtils | |||
| from .utils import GatewayEnumUtils | |||
|
|
|||
| BASE_GATEWAY_VERSION="1.0.0" | |||
There was a problem hiding this comment.
This has nothing to do with protobuf cleanup and it removes an existing behavior. As we broke the protocol between the CLI and the gateway in version 1.0.0 we added this test to give an error in case the user try to mix pre-1.0.0 version with a 1.0.0 one. This combination wouldn't work so we wanted to give a clear message about it. If you think this should be removed you need to discuss it with @caroav . It can't be done as part of a cleanup.
There was a problem hiding this comment.
@gbregman thank you for review.
I was not sure about the functionality of BASE_GATEWAY_VERSION or about its value.
I could imagine a CLI logic that rejects interaction with gateways reporting incompatible versions. For example, verifying compatibility behind the scenes during the handshake before executing the requested CLI command, such as "add namespace". However, in the current implementation, where it's used as a warning in the gw_get_info function, it seems odd.
Regarding the value, why don't we just compare the gateway version with CLI one, why this constant is required?
Please share you thoughts.
There was a problem hiding this comment.
@baum I suggest to detach this from the proto-buf cleanup PR. Then we can continue discussing this in another issue. Otherwise this discussion will hold this PR in the air for too long.
There was a problem hiding this comment.
@caroav would it be acceptable
- in gw_get_info function, restore the compare logic, however compare gateway version to CLI one
- remove the constant
There was a problem hiding this comment.
@baum we do compare the CLI and gw versions and issue a warning if they don't match. Anyway we needed something which will work even in cases there CLI-gw protocol is broken. So, I made sure the fewtching of the version still works after the protocol changes so we could issue the warning about the broken protocol.
baum
force-pushed
the
proto_cleanup
branch
2 times, most recently
from
94526c5
to
d36c8cc
Compare
baum
force-pushed
the
proto_cleanup
branch
2 times, most recently
from
fb0c499
to
663f806
Compare
- minor trtype cleanup - gw_get_info compare with CLI version Signed-off-by: Alexander Indenbaum <[email protected]>
| @@ -229,15 +227,6 @@ def get_actions(act_list): | |||
| acts += ", '" + a["name"] + "'" | |||
| return acts[2:] | |||
|
|
|||
| def format_adrfam(self, adrfam): | |||
There was a problem hiding this comment.
@baum why did you remove this formatting? This has nothing to do with protobuf cleanup. I wanted the output to have IPv4 and IPv6 which is the normal way to display address families. I don't understand why you insist on changing that.
No description provided.