openconfig / ygnmi

A Go gNMI client library based on ygot-generated code
Apache License 2.0
12 stars 4 forks source link

`SetBatch` doesn't allow each Update/Delete/Replace to have different Option arguments #83

Open wenovus opened 1 year ago

wenovus commented 1 year ago

The Option argument is only on SetBatch.Set, so it applies for all paths in the SetRequest:

func (sb *SetBatch) Set(ctx context.Context, c *Client, opts ...Option) (*Result, error) {
    req := &gpb.SetRequest{}
    for _, op := range sb.ops {
        path, err := resolvePath(op.path)
        if err != nil {
            return nil, err
        }
        if err := populateSetRequest(req, path, op.val, op.mode, op.config, opts...); err != nil {
            return nil, err
        }
    }
...

Since it's referenced in the for loop, these options are clearly per-path (e.g. ygnmi.WithSetFallbackEncoding() is a per-path option) but the current API doesn't allow for different options for different paths.

If we do move the option to the individual batch calls, however, there may be downstream implications for ondatra: https://github.com/openconfig/ondatra/blob/3b28896095b89670014dfb197224a83a50a9b5f2/gnmi/gnmi.go#L372-L381

So the root problem seems to be some of the options are per-client, while for others it is per-path.

I would like to break Ondatra's API to allow for a per-path behaviour. The use case is to Batch set CLI+OC in the same SetRequest.

@DanG100 Have any thoughts on how to support this?

wenovus commented 1 year ago

Actually for the CLI+OC use case the support for this is likely to be different (probably just a special case), so ygnmi.WithSetFallbackEncoding() is not necessary. However, since supporting this would break the API it's probably better to do this earlier than later to allow better Batch support.

DanG100 commented 1 year ago

Is there a use case for this? I get that in theory this might be nice, but a user could a also create 2 set requests.

wenovus commented 1 year ago

Is there a use case for this? I get that in theory this might be nice, but a user could a also create 2 set requests.

There is no use case right now for it. I think we can park this item. If the user wants transactional semantics then a single setrequest would be necessary.