open-traffic-generator / snappi

Open Traffic Generator SDK in Python and Go
MIT License
77 stars 7 forks source link

Has method is missing for some fields (ex - ports, flows etc. ) which are not required #117

Open shramroy opened 3 years ago

shramroy commented 3 years ago

Has method is missing for the below mentioned fields.


type Config interface {
    Msg() *snappipb.Config
    SetMsg(*snappipb.Config) Config
    ToPbText() string
    ToYaml() string
    ToJson() string
    FromPbText(value string) error
    FromYaml(value string) error
    FromJson(value string) error
    Validate(defaults ...bool) error
    validateObj(set_default bool)
    setDefault()
    Ports() ConfigPortIter
    Lags() ConfigLagIter
    Layer1() ConfigLayer1Iter
    Captures() ConfigCaptureIter
    Devices() ConfigDeviceIter
    Flows() ConfigFlowIter
    Events() Event
    HasEvents() bool
    Options() ConfigOptions
    HasOptions() bool
}
ashutshkumr commented 3 years ago

@ajbalogh could you please take a look ?

ajbalogh commented 3 years ago

The Has method will only be generated for optional fields. Per the otg.proto those fields are not optional so no Has method is generated for it.

  repeated Port ports = 1 [
    (fld_meta).description = "The ports that will be configured on the traffic generator."
  ];

  repeated Lag lags = 2 [
    (fld_meta).description = "The LAGs that will be configured on the traffic generator."
  ];

  repeated Layer1 layer1 = 3 [
    (fld_meta).description = "The layer1 settings that will be configured on the traffic generator."
  ];

  repeated Capture captures = 4 [
    (fld_meta).description = "The capture settings that will be configured on the traffic generator."
  ];

  repeated Device devices = 5 [
    (fld_meta).description = "The emulated devices that will be configured on the traffic generator.\nEach device contains configurations for network interfaces and \nprotocols running on top of those interfaces."
  ];

  repeated Flow flows = 6 [
    (fld_meta).description = "The flows that will be configured on the traffic generator."
  ];
ashutshkumr commented 3 years ago

I think optional is needed only for basic data types. For complex data types like objects or lists, protobuf by default generates those fields as pointers (i.e. optional).

type Config struct {
    state         protoimpl.MessageState
    sizeCache     protoimpl.SizeCache
    unknownFields protoimpl.UnknownFields

    Ports    []*Port        `protobuf:"bytes,1,rep,name=ports,proto3" json:"ports,omitempty"`
    Lags     []*Lag         `protobuf:"bytes,2,rep,name=lags,proto3" json:"lags,omitempty"`
    Layer1   []*Layer1      `protobuf:"bytes,3,rep,name=layer1,proto3" json:"layer1,omitempty"`
    Captures []*Capture     `protobuf:"bytes,4,rep,name=captures,proto3" json:"captures,omitempty"`
    Devices  []*Device      `protobuf:"bytes,5,rep,name=devices,proto3" json:"devices,omitempty"`
    Flows    []*Flow        `protobuf:"bytes,6,rep,name=flows,proto3" json:"flows,omitempty"`
    Events   *Event         `protobuf:"bytes,7,opt,name=events,proto3,oneof" json:"events,omitempty"`
    Options  *ConfigOptions `protobuf:"bytes,8,opt,name=options,proto3,oneof" json:"options,omitempty"`
}

If not this, how should a user check whether a config has ports or flows without really initializing them ?

ajbalogh commented 3 years ago

A HasPorts() method appears to be redundant as the base Ports protobuf field is wrapped in a PortsIter object. A user can then just get the length of the PortsIter.Items to determine whether or not any Port/Flow objects exist - why is the initialization of an empty array a concern?