Skip to content
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

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

Open
shramroy opened this issue Oct 6, 2021 · 4 comments
Assignees

Comments

@shramroy
Copy link
Contributor

shramroy commented Oct 6, 2021

Has method is missing for the below mentioned fields.

  • ports, flows, lags, layer1, capture, device.
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
Copy link
Contributor

@ajbalogh could you please take a look ?

@ajbalogh
Copy link
Contributor

ajbalogh commented Oct 6, 2021

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."
  ];

@ajbalogh ajbalogh closed this as completed Oct 6, 2021
@ashutshkumr
Copy link
Contributor

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 ?

@ashutshkumr ashutshkumr reopened this Oct 6, 2021
@ajbalogh
Copy link
Contributor

ajbalogh commented Oct 6, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants